Bug 174691

Summary: Hook up ITP quirks to the needsSiteSpecificQuirks setting
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, buildbot, commit-queue, darin, dbates, ggaren, japhet, jeremyj-wk, sam
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 174533, 174661    
Bug Blocks: 188710    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2017-07-20 14:10:49 PDT
Hook up ITP quirks to the needsSiteSpecificQuirks setting.
Comment 1 Chris Dumez 2017-07-20 15:08:10 PDT
Created attachment 316030 [details]
Patch
Comment 2 Chris Dumez 2017-07-20 16:05:01 PDT
Created attachment 316036 [details]
Patch
Comment 3 Jeremy Jones 2017-07-20 16:41:55 PDT
Comment on attachment 316036 [details]
Patch

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

provisional r+

> Source/WebCore/ChangeLog:8
> +        Hook up ITP quirks to the needsSiteSpecificQuirks setting to make it easier to

nit: easier for
Comment 4 Chris Dumez 2017-07-20 16:57:27 PDT
Created attachment 316043 [details]
Patch
Comment 5 Darin Adler 2017-07-20 18:11:38 PDT
Comment on attachment 316043 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:65
> +#if PLATFORM(IOS)
> +    UNUSED_PARAM(page);
> +
> +    // There is currently no way to toggle the needsSiteSpecificQuirks setting on iOS so we always enable
> +    // the site-specific quirks on iOS.
> +    return true;
> +#else

It is really strange that this logic is here rather than in Settings.
Comment 6 Chris Dumez 2017-07-20 18:18:51 PDT
(In reply to Darin Adler from comment #5)
> Comment on attachment 316043 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316043&action=review
> 
> > Source/WebCore/loader/ResourceLoadObserver.cpp:65
> > +#if PLATFORM(IOS)
> > +    UNUSED_PARAM(page);
> > +
> > +    // There is currently no way to toggle the needsSiteSpecificQuirks setting on iOS so we always enable
> > +    // the site-specific quirks on iOS.
> > +    return true;
> > +#else
> 
> It is really strange that this logic is here rather than in Settings.

The setting is disabled on iOS. However, I need the ITP quirk on iOS as well. We could enable the setting on iOS but then it would enable other quirks than ITP on iOS, which would be risky at this point.
Comment 7 WebKit Commit Bot 2017-07-20 18:39:01 PDT
Comment on attachment 316043 [details]
Patch

Clearing flags on attachment: 316043

Committed r219711: <http://trac.webkit.org/changeset/219711>
Comment 8 WebKit Commit Bot 2017-07-20 18:39:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Darin Adler 2017-07-21 09:28:09 PDT
Comment on attachment 316043 [details]
Patch

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

>>> Source/WebCore/loader/ResourceLoadObserver.cpp:65
>>> +#else
>> 
>> It is really strange that this logic is here rather than in Settings.
> 
> The setting is disabled on iOS. However, I need the ITP quirk on iOS as well. We could enable the setting on iOS but then it would enable other quirks than ITP on iOS, which would be risky at this point.

Ah, so site-specific quirks have been non-iOS-only up until now. That seems sloppy and seems worth cleaning up when we have a chance.
Comment 10 Brent Fulgham 2017-10-24 09:04:20 PDT
<rdar://problem/33465715>