Bug 174662

Summary: [Win] Implement Authentication dialog in MiniBrowser
Product: WebKit Reporter: Basuke Suzuki <Basuke.Suzuki>
Component: Tools / TestsAssignee: Basuke Suzuki <Basuke.Suzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, Basuke.Suzuki, bfulgham, buildbot, commit-queue, don.olmstead, lforschler, pvollan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=189846
Attachments:
Description Flags
Implemented
none
Fix style
none
Fix style
none
Archive of layout-test-results from ews124 for ios-simulator-wk2 none

Description Basuke Suzuki 2017-07-19 17:58:21 PDT
There's no authentication dialog for Windows MiniBrowser.
Comment 1 Basuke Suzuki 2017-07-20 10:19:03 PDT
Created attachment 315997 [details]
Implemented
Comment 2 Build Bot 2017-07-20 10:21:36 PDT
Attachment 315997 [details] did not pass style-queue:


ERROR: Tools/MiniBrowser/win/Common.cpp:671:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Tools/MiniBrowser/win/Common.cpp:681:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:1:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Tools/MiniBrowser/win/ResourceLoadDelegate.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/MiniBrowser/win/ResourceLoadDelegate.cpp:97:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Tools/MiniBrowser/win/ResourceLoadDelegate.cpp:107:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 7 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Basuke Suzuki 2017-07-20 11:32:00 PDT
Created attachment 316005 [details]
Fix style
Comment 4 Basuke Suzuki 2017-07-20 11:33:11 PDT
(In reply to Build Bot from comment #2)
> ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0:  No copyright
> message found.  You should have a line: "Copyright [year] <Copyright Owner>"
> [legal/copyright] [5]
> ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:1:  Should have a
> space between // and comment  [whitespace/comments] [4]

This file is the one VisualStudio generates.
Comment 5 Basuke Suzuki 2017-07-20 11:34:26 PDT
Created attachment 316007 [details]
Fix style
Comment 6 Build Bot 2017-07-20 11:36:31 PDT
Attachment 316007 [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]
ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:1:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Build Bot 2017-07-20 13:00:50 PDT
Comment on attachment 316007 [details]
Fix style

Attachment 316007 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4156792

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html
Comment 8 Build Bot 2017-07-20 13:00:51 PDT
Created attachment 316018 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 9 Basuke Suzuki 2017-07-20 14:19:08 PDT
I have no idea why my change for Windows app break ios-sim layout test. Also I don't want to fix generated file because next modification causes that change useless.
Comment 10 Alex Christensen 2017-07-21 14:07:19 PDT
Comment on attachment 316007 [details]
Fix style

View in context: https://bugs.webkit.org/attachment.cgi?id=316007&action=review

iOS test failure unrelated

> Tools/MiniBrowser/win/ResourceLoadDelegate.cpp:40
> +extern HRESULT DisplayAuthDialog(std::wstring& username, std::wstring& password);

This should go in a header
Comment 11 Basuke Suzuki 2017-07-21 17:19:49 PDT
(In reply to Alex Christensen from comment #10)
> Comment on attachment 316007 [details]
> Fix style
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316007&action=review
> 
> iOS test failure unrelated
> 
> > Tools/MiniBrowser/win/ResourceLoadDelegate.cpp:40
> > +extern HRESULT DisplayAuthDialog(std::wstring& username, std::wstring& password);
> 
> This should go in a header

I agree with that, but basic architecture of this app is very new to me which is very confusing. The "Common.cpp" file has no header file and directly included from "WinMain.cpp" so that no chance to get the definition easily. Other references to those defined method seems to be called inside Common.cpp as the result of event.

I am very new to Windows app programming and have no idea with this structure.
Comment 12 Alex Christensen 2017-07-24 11:07:31 PDT
Comment on attachment 316007 [details]
Fix style

MiniBrowser isn't exactly the most well-organized project anyways.  Let's just land this.
Comment 13 WebKit Commit Bot 2017-07-24 11:35:02 PDT
Comment on attachment 316007 [details]
Fix style

Clearing flags on attachment: 316007

Committed r219830: <http://trac.webkit.org/changeset/219830>
Comment 14 WebKit Commit Bot 2017-07-24 11:35:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Basuke Suzuki 2017-07-24 12:31:03 PDT
(In reply to Alex Christensen from comment #12)
> Comment on attachment 316007 [details]
> Fix style
> 
> MiniBrowser isn't exactly the most well-organized project anyways.  Let's
> just land this.

Thanks!