| Summary: | Add an integer identifier field to AuthenticationChallengeBase | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||||
| Component: | Page Loading | Assignee: | Brady Eidson <beidson> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, andersca, ap, danw, fishd, gtk-ews, gustavo, mrobinson, rakuco, sam, webkit-ews, webkit.review.bot, xan.lopez | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 102592 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Brady Eidson
2012-11-17 11:42:09 PST
Created attachment 174824 [details]
Patch v1
Attachment 174824 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/network/cf/AuthenticationChallenge.h:52: The parameter name "protectionSpace" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/AuthenticationChallenge.h:52: The parameter name "response" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/AuthenticationChallenge.h:52: The parameter name "error" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 3 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 174824 [details] Patch v1 Attachment 174824 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14877336 Comment on attachment 174824 [details] Patch v1 Attachment 174824 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14859835 Comment on attachment 174824 [details] Patch v1 Attachment 174824 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14875419 Created attachment 174827 [details]
Patch v2 - Probably need one more round at EWS
One more shot at EWS to make sure qt/soup/win don't need changes within WebCore. Attachment 174827 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/network/qt/AuthenticationChallenge.h:43: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/platform/network/soup/AuthenticationChallenge.h:45: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/platform/network/win/AuthenticationChallenge.h:41: Tab found; better to use spaces [whitespace/tab] [1]
Total errors found: 3 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Replaced tabs locally. As long as the rest of EWS passes, I won't reupload for that. Comment on attachment 174827 [details] Patch v2 - Probably need one more round at EWS Attachment 174827 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14876374 Created attachment 174829 [details]
Patch v3 - Hopefully last shot at EWS
Attachment 174829 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/network/cf/AuthenticationCF.cpp:56: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/network/cf/AuthenticationCF.cpp:57: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/network/cf/AuthenticationCF.cpp:64: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/platform/network/cf/AuthenticationCF.cpp:77: Tab found; better to use spaces [whitespace/tab] [1]
Total errors found: 4 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 174829 [details] Patch v3 - Hopefully last shot at EWS Attachment 174829 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14879295 Created attachment 174830 [details]
Patch v4 - Come on Windows, you can do it.
Attachment 174830 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/network/cf/AuthenticationCF.cpp:56: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/network/cf/AuthenticationCF.cpp:57: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit/win/WebURLAuthenticationChallenge.cpp:151: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Total errors found: 3 in 14 files
If any of these errors are false positives, please file a bug against check-webkit-style.
With EWS finally satisfied, marking r? Comment on attachment 174830 [details] Patch v4 - Come on Windows, you can do it. View in context: https://bugs.webkit.org/attachment.cgi?id=174830&action=review > Source/WebCore/platform/network/AuthenticationChallengeBase.cpp:50 > + , m_identifier(0) Seems this constructor should take the identifier as an argument. > Source/WebCore/platform/network/AuthenticationChallengeBase.cpp:87 > + m_identifier = 0; Why need this one field be zeroed out explicitly, but not others, such as m_error? (In reply to comment #17) > (From update of attachment 174830 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174830&action=review > > > Source/WebCore/platform/network/AuthenticationChallengeBase.cpp:50 > > + , m_identifier(0) > > Seems this constructor should take the identifier as an argument. Eventually it might. But for now the usefulness of the identifier is tied up directly with creation from a platform authentication challenge. This will be clear in https://bugs.webkit.org/show_bug.cgi?id=102592 > > > Source/WebCore/platform/network/AuthenticationChallengeBase.cpp:87 > > + m_identifier = 0; > > Why need this one field be zeroed out explicitly, but not others, such as m_error? I'll leave this m_identifier = 0; out of this patch and re-add it with the patch upcoming in https://bugs.webkit.org/show_bug.cgi?id=102592 along with a more thorough nulling-out. Comment on attachment 174830 [details] Patch v4 - Come on Windows, you can do it. View in context: https://bugs.webkit.org/attachment.cgi?id=174830&action=review > Source/WebCore/platform/network/AuthenticationChallengeBase.cpp:35 > + , m_identifier(0) Historically, the Chromium port has made an effort to keep identifiers like this out of WebCore because they are usually a concern of multiprocess code an not a concern of WebCore. Would it be possible to keep these WebKit2-only concerns in the WebKit2 layer so as not to add complexity to ports that don't care to use WebKit2? (In reply to comment #20) > (From update of attachment 174830 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174830&action=review > > > Source/WebCore/platform/network/AuthenticationChallengeBase.cpp:35 > > + , m_identifier(0) > > Historically, the Chromium port has made an effort to keep identifiers like this out of WebCore because they are usually a concern of multiprocess code an not a concern of WebCore. We've done a lot of the same in WebKit2. > Would it be possible to keep these WebKit2-only concerns in the WebKit2 layer so as not to add complexity to ports that don't care to use WebKit2? Each situation should probably be viewed on its own merits. In WebKit2's CoreIPC we commonly pass WebCore types over the wire, and there's been plenty of periodic work to make changes in WebCore to support this. The WebCore/platform directly is both an interface layer to adapt platforms to WebCore and a support layer to provide those ports the support they need for their goals. Especially since the frequency of AuthenticationChallenges is so low and there's basically no risk for performance or memory usage issues, it seemed reasonable to advance in this manner. Especially compared to some of the much larger and more turbulent refactorings in the past that were solely to support one port's IPC architecture, I'm not sure what the big deal is about this one. If you feel strongly about it I can revisit and put the identifier member in each of the relevant ports AuthenticationChallenge.h/cpps. But I'm not sure how that is better. > I'm not sure what the big deal is about this one.
Oh, I don't think it's such a big deal. It's just a design pattern that we consciously try to avoid, and I wanted to make sure we were all on the same page. It sounds like you agree with the principle in general.
(In reply to comment #22) > > I'm not sure what the big deal is about this one. > > Oh, I don't think it's such a big deal. It's just a design pattern that we consciously try to avoid, and I wanted to make sure we were all on the same page. It sounds like you agree with the principle in general. I think we agree in general. > Historically, the Chromium port has made an effort to keep identifiers like this out of WebCore because they are usually a concern of multiprocess code an not a concern of WebCore. Would it be possible to keep these WebKit2-only concerns in the WebKit2 layer so as not to add complexity to ports that don't care to use WebKit2?
Yes, I agree. In fact, WebKit2 already uses a better technique when exchanging AuthenticationChallenges between WebProcess and UI Process. A transient ID is generated just for IPC, and is not part of AuthenticationChallenge.
I'll work on a patch that removes the identifier from AuthenticationChallenge. This patch will also make NetworkProcess IPC more efficient, as we won't be sending an unchanged AuthenticationChallenge in responses.
|