Bug 186134

Created attachment 341654 [details] Patch

Could anyone review this patch?

Comment on attachment 341654 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341654&action=review Can you please rebase the patch so it applies cleanly to the tree? We can't approve it if we don't know if it breaks any builds. > Tools/ChangeLog:8 > + It should not assume there is only one main window and one browser It should not BE assumed there is ...

Thank you, Brent. (In reply to Brent Fulgham from comment #3) > Can you please rebase the patch so it applies cleanly to the tree? We can't > approve it if we don't know if it breaks any builds. This patch is up-to-date. I don't know the reason why MiniBrowserLib.rc can be merged. I'm guessing that's because I was using Cygwin Git to create the patch. I will try native svn or git. > > Tools/ChangeLog:8 > > + It should not assume there is only one main window and one browser > > It should not BE assumed there is ... Thanks. I will fix.

Created attachment 341710 [details] Patch

Attachment 341710 [details] did not pass style-queue: ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 341710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341710&action=review r=me > Tools/MiniBrowser/win/MainWindow.cpp:136 > + MainWindow* thiz = reinterpret_cast<MainWindow*>(GetWindowLongPtr(hWnd, GWLP_USERDATA)); Is it possible for thiz to be null? Likewise, is it possible for thiz->browserWindow() to return a nullptr? If not, why isn't browserWindow() returning a reference? > Tools/MiniBrowser/win/MainWindow.cpp:448 > + *(reinterpret_cast<LPWORD>(strPtr)) = INTERNET_MAX_URL_LENGTH; Could this be done with some kind of modern VisualStudio BSTR type? This seems so gross and likely to be confusing to someone unfamiliar with this. I realize this is just existing code you moved, but it's still not very nice.

Comment on attachment 341710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341710&action=review Thank you for r+, Brent. >> Tools/MiniBrowser/win/MainWindow.cpp:136 >> + MainWindow* thiz = reinterpret_cast<MainWindow*>(GetWindowLongPtr(hWnd, GWLP_USERDATA)); > > Is it possible for thiz to be null? > > Likewise, is it possible for thiz->browserWindow() to return a nullptr? > > If not, why isn't browserWindow() returning a reference? Agreed. I will fix. >> Tools/MiniBrowser/win/MainWindow.cpp:448 >> + *(reinterpret_cast<LPWORD>(strPtr)) = INTERNET_MAX_URL_LENGTH; > > Could this be done with some kind of modern VisualStudio BSTR type? This seems so gross and likely to be confusing to someone unfamiliar with this. > > I realize this is just existing code you moved, but it's still not very nice. Year. it's too complicated code just to get the text of an edit control. That is caused by the spec of EM_GETLINE. I think I can use GetWindowText. I will try. EM_GETLINE message (Windows) https://msdn.microsoft.com/ja-jp/library/windows/desktop/bb761584(v=vs.85).aspx GetWindowText function (Windows) https://msdn.microsoft.com/ja-jp/library/windows/desktop/ms633520(v=vs.85).aspx

Created attachment 341885 [details] Patch

Attachment 341885 [details] did not pass style-queue: ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Committed r232458: <https://trac.webkit.org/changeset/232458>

<rdar://problem/40761065>