Bug 80135 – [BlackBerry] http authenticate dialog popup only once no matter authentication pass or fail

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Jonathan Dong]

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.

[Jonathan Dong]

Created attachment 133403 [details]
Patch

[Rob Buis]

Comment on attachment 133403 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 133403 [details]
Patch

Clearing flags on attachment: 133403

Committed r111810: <http://trac.webkit.org/changeset/111810>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jonathan Dong]

Reopening to attach new patch.

[Jonathan Dong]

Created attachment 133796 [details]
Patch

[Jonathan Dong]

hi Rob, do you have any comment on this? :)

[Rob Buis]

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.

[Jonathan Dong]

Created attachment 134575 [details]
Patch

[Rob Buis]

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.

[Jonathan Dong]

Created attachment 134955 [details]
Patch

[Rob Buis]

Comment on attachment 134955 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 134955 [details]
Patch

Clearing flags on attachment: 134955

Committed r112794: <http://trac.webkit.org/changeset/112794>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joe Mason]

Re-opening: these changes broke digest auth.  Going to revert them until we can figure out the problem.

[Joe Mason]

Created attachment 138464 [details]
revert patch

[George Staikos]

Comment on attachment 138464 [details]
revert patch

Why remove the tests?

[Joe Mason]

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

[Joe Mason]

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.

[Joe Mason]

Created attachment 138570 [details]
revert all but the tests

Updated patch that reverts all but the tests.

[WebKit Review Bot]

Comment on attachment 138570 [details]
revert all but the tests

Clearing flags on attachment: 138570

Committed r115104: <http://trac.webkit.org/changeset/115104>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joe Mason]

No, those were reverts, so this is still open.

[Jonathan Dong]

Created attachment 144364 [details]
Patch

[Rob Buis]

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?

[Jonathan Dong]

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.

[Jonathan Dong]

Created attachment 144426 [details]
Patch

[WebKit Review Bot]

Comment on attachment 144426 [details]
Patch

Clearing flags on attachment: 144426

Committed r118719: <http://trac.webkit.org/changeset/118719>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.