Bug 186134 – [Win][MiniBrowser] Remove gMiniBrowser global variable

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Fujii Hironori]

[Win][MiniBrowser] Remove gMiniBrowser global variable

[Fujii Hironori]

Created attachment 341654 [details]
Patch

[Fujii Hironori]

Could anyone review this patch?

[Brent Fulgham]

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 ...

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 341710 [details]
Patch

[EWS Watchlist]

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.

[Brent Fulgham]

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.

[Fujii Hironori]

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

[Fujii Hironori]

Created attachment 341885 [details]
Patch

[EWS Watchlist]

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.

[Fujii Hironori]

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

[Radar WebKit Bug Importer]

<rdar://problem/40761065>