Bug 147678 – REGRESSION (r185111): Clicking phone numbers doesn't prompt to call sometimes

• Component: WebKit2
• Status: RESOLVED FIXED

[Daniel Bates]

Created attachment 258262 [details]
Example

<rdar://problem/21827815>

Following the patch for bug #145280, clicking on phone links (tel URLs) may not work. On iOS, you can use the attached example page to observe this issue by performing the following:

1. Open example.html in MobileSafari.
2. Click the button "Call [phone] in nested zero timer callback".
3. Quit and launch MobileSafari again.
4. Click the button "Call [phone] in nested zero timer callback".

Then nothing will happen. Compare the behavior after step 4 with the behavior observed in step 2.

[Daniel Bates]

Created attachment 258265 [details]
Patch and unit test

[Brady Eidson]

Comment on attachment 258265 [details]
Patch and unit test

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

> Source/WebCore/loader/HistoryController.cpp:196
> +        if (DocumentLoader* documentLoader = document.loader())
> +            item->setShouldOpenExternalURLsPolicy(documentLoader->shouldOpenExternalURLsPolicyToPropagate());
> +

This when saving document state...

> Source/WebCore/loader/HistoryController.cpp:237
> +    m_frame.loader().documentLoader()->setShouldOpenExternalURLsPolicy(m_currentItem->shouldOpenExternalURLsPolicy());
> +

...combined with this seems like it could be problematic, because if it is possible to have "saved document state" to a history item without a DocumentLoader, therefore the policy was not set, that means it's possible to pull an invalid policy out of the history item later.

[Brady Eidson]

(In reply to comment #2)
> Comment on attachment 258265 [details]
> Patch and unit test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258265&action=review
> 
> > Source/WebCore/loader/HistoryController.cpp:196
> > +        if (DocumentLoader* documentLoader = document.loader())
> > +            item->setShouldOpenExternalURLsPolicy(documentLoader->shouldOpenExternalURLsPolicyToPropagate());
> > +
> 
> This when saving document state...
> 
> > Source/WebCore/loader/HistoryController.cpp:237
> > +    m_frame.loader().documentLoader()->setShouldOpenExternalURLsPolicy(m_currentItem->shouldOpenExternalURLsPolicy());
> > +
> 
> ...combined with this seems like it could be problematic, because if it is
> possible to have "saved document state" to a history item without a
> DocumentLoader, therefore the policy was not set, that means it's possible
> to pull an invalid policy out of the history item later.

Looking at Document::loader(), if the Frame returns the Document, then the DocumentLoader will almost certainly *not* be null. The only cases where it might be null are cases where we are already in a grotesquely inconsistent state, so I'm not worried about this any longer.

[Brady Eidson]

Comment on attachment 258265 [details]
Patch and unit test

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

Looks good, with one requested change.

> Source/WebCore/loader/NavigationAction.h:47
> +    NavigationAction(const ResourceRequest&, FrameLoadType, bool isFormSubmission, ShouldOpenExternalURLsPolicy);

A cleanup project of mine very soon is going to be *reducing* the number of NavigationAction constructors (ideally to one), forcing everybody who ever creates one to think about every argument critically.

I'd rather see the three call sites of this new constructor pass in the values you pass in this constructor's implementation, instead of adding this constructor now just to be removed soon.

[Daniel Bates]

Created attachment 258267 [details]
Patch and unit test

[Daniel Bates]

Committed r187962: <http://trac.webkit.org/changeset/187962>