Bug 80135 - [BlackBerry] http authenticate dialog popup only once no matter authentication pass or fail
Summary: [BlackBerry] http authenticate dialog popup only once no matter authenticatio...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 84579
  Show dependency treegraph
 
Reported: 2012-03-02 02:14 PST by Jonathan Dong
Modified: 2012-05-28 20:49 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.90 KB, patch)
2012-03-22 18:06 PDT, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch (3.62 KB, patch)
2012-03-26 07:01 PDT, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch (3.57 KB, patch)
2012-03-29 07:14 PDT, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch (8.49 KB, patch)
2012-03-31 05:54 PDT, Jonathan Dong
no flags Details | Formatted Diff | Diff
revert patch (18.45 KB, patch)
2012-04-23 17:13 PDT, Joe Mason
no flags Details | Formatted Diff | Diff
revert all but tests (14.53 KB, patch)
2012-04-24 07:16 PDT, Joe Mason
no flags Details | Formatted Diff | Diff
revert all but the tests (14.22 KB, patch)
2012-04-24 08:38 PDT, Joe Mason
no flags Details | Formatted Diff | Diff
Patch (13.47 KB, patch)
2012-05-28 08:12 PDT, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch (14.74 KB, patch)
2012-05-28 19:32 PDT, Jonathan Dong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Dong 2012-03-02 02:14:46 PST
if wrong credential information is provided by user for the first time, some site would return 401 again to challenge user to re-enter the credential, until user correctly login or press cancel.

According to our implementation for now, NetworkJob would not start a new request when the m_currentWebChallenge in its related resourceHandle is not null.
It won't send out a new request when user leaves both username and password empty then press enter.
Comment 1 Jonathan Dong 2012-03-22 18:06:12 PDT
Created attachment 133403 [details]
Patch
Comment 2 Rob Buis 2012-03-22 18:38:26 PDT
Comment on attachment 133403 [details]
Patch

Looks good.
Comment 3 WebKit Review Bot 2012-03-22 19:24:15 PDT
Comment on attachment 133403 [details]
Patch

Clearing flags on attachment: 133403

Committed r111810: <http://trac.webkit.org/changeset/111810>
Comment 4 WebKit Review Bot 2012-03-22 19:24:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Jonathan Dong 2012-03-26 07:01:43 PDT
Reopening to attach new patch.
Comment 6 Jonathan Dong 2012-03-26 07:01:51 PDT
Created attachment 133796 [details]
Patch
Comment 7 Jonathan Dong 2012-03-29 06:33:42 PDT
hi Rob, do you have any comment on this? :)
Comment 8 Rob Buis 2012-03-29 06:50:11 PDT
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.
Comment 9 Jonathan Dong 2012-03-29 07:14:37 PDT
Created attachment 134575 [details]
Patch
Comment 10 Rob Buis 2012-03-29 07:20:58 PDT
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.
Comment 11 Jonathan Dong 2012-03-31 05:54:19 PDT
Created attachment 134955 [details]
Patch
Comment 12 Rob Buis 2012-03-31 05:59:16 PDT
Comment on attachment 134955 [details]
Patch

Looks good.
Comment 13 WebKit Review Bot 2012-03-31 06:34:21 PDT
Comment on attachment 134955 [details]
Patch

Clearing flags on attachment: 134955

Committed r112794: <http://trac.webkit.org/changeset/112794>
Comment 14 WebKit Review Bot 2012-03-31 06:34:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Joe Mason 2012-04-23 17:13:02 PDT
Re-opening: these changes broke digest auth.  Going to revert them until we can figure out the problem.
Comment 16 Joe Mason 2012-04-23 17:13:37 PDT
Created attachment 138464 [details]
revert patch
Comment 17 George Staikos 2012-04-23 17:51:47 PDT
Comment on attachment 138464 [details]
revert patch

Why remove the tests?
Comment 18 Joe Mason 2012-04-24 07:11:16 PDT
(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.
Comment 19 Joe Mason 2012-04-24 07:16:12 PDT
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.
Comment 20 Joe Mason 2012-04-24 08:38:17 PDT
Created attachment 138570 [details]
revert all but the tests

Updated patch that reverts all but the tests.
Comment 21 WebKit Review Bot 2012-04-24 13:53:08 PDT
Comment on attachment 138570 [details]
revert all but the tests

Clearing flags on attachment: 138570

Committed r115104: <http://trac.webkit.org/changeset/115104>
Comment 22 WebKit Review Bot 2012-04-24 13:53:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Joe Mason 2012-04-24 13:58:48 PDT
No, those were reverts, so this is still open.
Comment 24 Jonathan Dong 2012-05-28 08:12:39 PDT
Created attachment 144364 [details]
Patch
Comment 25 Rob Buis 2012-05-28 11:19:18 PDT
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 26 Jonathan Dong 2012-05-28 19:29:20 PDT
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.
Comment 27 Jonathan Dong 2012-05-28 19:32:44 PDT
Created attachment 144426 [details]
Patch
Comment 28 WebKit Review Bot 2012-05-28 20:49:49 PDT
Comment on attachment 144426 [details]
Patch

Clearing flags on attachment: 144426

Committed r118719: <http://trac.webkit.org/changeset/118719>
Comment 29 WebKit Review Bot 2012-05-28 20:49:55 PDT
All reviewed patches have been landed.  Closing bug.