Summary: | REGRESSION (r185111): Clicking phone numbers doesn't prompt to call sometimes | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||||
Component: | WebKit2 | Assignee: | Daniel Bates <dbates> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aestes, andersca, ap, beidson, cdumez, commit-queue, ddkilzer, japhet, mitz | ||||||||
Priority: | P1 | Keywords: | InRadar, Regression | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | 145280 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Daniel Bates
2015-08-04 21:25:14 PDT
Created attachment 258265 [details]
Patch and unit test
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. (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. 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. Created attachment 258267 [details]
Patch and unit test
Committed r187962: <http://trac.webkit.org/changeset/187962> |