Bug 186134 - [Win][MiniBrowser] Remove gMiniBrowser global variable
Summary: [Win][MiniBrowser] Remove gMiniBrowser global variable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on:
Blocks: 184770
  Show dependency treegraph
 
Reported: 2018-05-31 02:31 PDT by Fujii Hironori
Modified: 2018-06-03 20:37 PDT (History)
6 users (show)

See Also:


Attachments
Patch (17.37 KB, patch)
2018-05-31 02:48 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (17.17 KB, patch)
2018-05-31 17:08 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (17.01 KB, patch)
2018-06-03 20:20 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2018-05-31 02:31:36 PDT
[Win][MiniBrowser] Remove gMiniBrowser global variable
Comment 1 Fujii Hironori 2018-05-31 02:48:16 PDT
Created attachment 341654 [details]
Patch
Comment 2 Fujii Hironori 2018-05-31 13:51:39 PDT
Could anyone review this patch?
Comment 3 Brent Fulgham 2018-05-31 14:02:35 PDT
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 ...
Comment 4 Fujii Hironori 2018-05-31 15:09:04 PDT
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.
Comment 5 Fujii Hironori 2018-05-31 17:08:40 PDT
Created attachment 341710 [details]
Patch
Comment 6 EWS Watchlist 2018-05-31 17:10:42 PDT
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 7 Brent Fulgham 2018-05-31 17:25:41 PDT
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 8 Fujii Hironori 2018-05-31 17:55:31 PDT
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
Comment 9 Fujii Hironori 2018-06-03 20:20:38 PDT
Created attachment 341885 [details]
Patch
Comment 10 EWS Watchlist 2018-06-03 20:23:15 PDT
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.
Comment 11 Fujii Hironori 2018-06-03 20:26:35 PDT
Committed r232458: <https://trac.webkit.org/changeset/232458>
Comment 12 Radar WebKit Bug Importer 2018-06-03 20:37:08 PDT
<rdar://problem/40761065>