Summary: | [BlackBerry] http authenticate dialog popup only once no matter authentication pass or fail | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Dong <jonathan.dong.webkit> | ||||||||||||||||||||
Component: | WebKit BlackBerry | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | charles.wei, joenotcharles, leo.yang, rwlbuis, tonikitoo, webkit.review.bot | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 84579 | ||||||||||||||||||||||
Attachments: |
|
Description
Jonathan Dong
2012-03-02 02:14:46 PST
Created attachment 133403 [details]
Patch
Comment on attachment 133403 [details]
Patch
Looks good.
Comment on attachment 133403 [details] Patch Clearing flags on attachment: 133403 Committed r111810: <http://trac.webkit.org/changeset/111810> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 133796 [details]
Patch
hi Rob, do you have any comment on this? :) Comment on attachment 133796 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133796&action=review > Source/WebCore/ChangeLog:8 > + Fixed a regression introduced by last patch to fix this PR (git SHA: d9a7ccc1a98e), I dont think this is useful for webkit community, to mention internal SHAs. I propose to do what chromium does, refer to our internal bug (like PR 10777), and in the internal bug you can refer to SHAs. However if you want to refer to a previous upstream webkit changeset you did (like r110000), that would be fine. Created attachment 134575 [details]
Patch
Comment on attachment 134575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134575&action=review Still some issues :) > Source/WebCore/ChangeLog:5 > + RIM PR: 145660 It is better to put this below the Reviewed By line. In fact cq+ might not work if it is strict about the format. > Source/WebCore/ChangeLog:9 > + Fixed a regression introduced by last patch to fix this PR (r111810), we should Better rephrase: Fixed a regression introduced by r111810, we should > Source/WebCore/ChangeLog:13 > + No new tests. Why no new tests? > Source/WebKit/blackberry/ChangeLog:9 > + Fixed a regression introduced by last patch to fix this PR (r111810), which Same as above. Created attachment 134955 [details]
Patch
Comment on attachment 134955 [details]
Patch
Looks good.
Comment on attachment 134955 [details] Patch Clearing flags on attachment: 134955 Committed r112794: <http://trac.webkit.org/changeset/112794> All reviewed patches have been landed. Closing bug. Re-opening: these changes broke digest auth. Going to revert them until we can figure out the problem. Created attachment 138464 [details]
revert patch
Comment on attachment 138464 [details]
revert patch
Why remove the tests?
(In reply to comment #17) > (From update of attachment 138464 [details]) > Why remove the tests? I just reverted the whole thing. This is "git revert" on all three patches, squashed. Created attachment 138552 [details]
revert all but tests
Here's the same patch without the tests removed. r+ whichever you prefer and r- the other one.
Created attachment 138570 [details]
revert all but the tests
Updated patch that reverts all but the tests.
Comment on attachment 138570 [details] revert all but the tests Clearing flags on attachment: 138570 Committed r115104: <http://trac.webkit.org/changeset/115104> All reviewed patches have been landed. Closing bug. No, those were reverts, so this is still open. Created attachment 144364 [details]
Patch
Comment on attachment 144364 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144364&action=review Looks good but please fix my issues before landing. > Source/WebCore/ChangeLog:9 > + issue for BlackBerry porting get identified and fixed. It is better to keep the original ChangeLog messages. Or keep them and append these two lines too. > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:130 > + return "<unknown>"; This needs to be implemented? If so add a TODO. > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:877 > + return true; Is this tested? Do you think we will need to change some test results? Comment on attachment 144364 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144364&action=review >> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:877 >> + return true; > > Is this tested? Do you think we will need to change some test results? yes it's been tested with http/tests/loading/basic-auth-resend-wrong-credentials.html and http/tests/loading/basic-credentials-sent-automatically.html I don't think we need to change test results because that the purpose of reverting this patch is for the infinite loop challenge when loading subresources, it has nothing to do with the DRT implementation. And the tests work fine for now. Created attachment 144426 [details]
Patch
Comment on attachment 144426 [details] Patch Clearing flags on attachment: 144426 Committed r118719: <http://trac.webkit.org/changeset/118719> All reviewed patches have been landed. Closing bug. |