Bug 145573

Summary: No need to guard the sizes attribute against PICTURE_SIZES
Product: WebKit Reporter: Dean Jackson <dino>
Component: New BugsAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, dino, esprehn+autocc, gyuyoung.kim, mmaxfield, ossy, webkit-bug-importer, yoav, zalan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch mmaxfield: review+

Description Dean Jackson 2015-06-02 15:15:40 PDT
No need to guard the sizes attribute against PICTURE_SIZES
Comment 1 Radar WebKit Bug Importer 2015-06-02 15:16:45 PDT
<rdar://problem/21210038>
Comment 2 Dean Jackson 2015-06-02 15:18:09 PDT
Created attachment 254111 [details]
Patch
Comment 3 Dean Jackson 2015-06-02 15:31:31 PDT
Committed r185130: <http://trac.webkit.org/changeset/185130>
Comment 4 Myles C. Maxfield 2015-06-02 15:31:44 PDT
Comment on attachment 254111 [details]
Patch

rs=me
Comment 5 Darin Adler 2015-06-03 13:34:59 PDT
Comment on attachment 254111 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        The PICTURE_SIZES feature flag doesn't need to be used to
> +        guard preloading of the sizes attribute.

I guess this should be obvious to me, but why?
Comment 6 Yoav Weiss 2015-06-03 13:42:10 PDT
The PICTURE_SIZES guard was put back in just on the currentSrc attribute implementation, due to https://bugs.webkit.org/show_bug.cgi?id=144095 and the fact that we'd have to remove currentSrc support unless the high-profile sites that use the old polyfill will update it soon.

Since the issue only involves the currentSrc attribute, the flag should only guard its implementation, and not the sizes attribute (despite the flag's name - it retained the name of the old flag, that guarded the entire feature).
Comment 7 Darin Adler 2015-06-03 13:48:03 PDT
OK. We should rename the flag unless it’s going away soon.
Comment 8 Csaba Osztrogonác 2015-06-12 04:53:39 PDT
(In reply to comment #3)
> Committed r185130: <http://trac.webkit.org/changeset/185130>

This change broke the !ENABLE(PICTURE_SIZES) build:

./../Source/WebCore/html/parser/HTMLPreloadScanner.cpp: In member function 'void WebCore::TokenPreloadScanner::StartTagScanner::processAttributes(const AttributeList&, WebCore::Document&)':
../../Source/WebCore/html/parser/HTMLPreloadScanner.cpp:115:46: error: 'm_sizesAttribute' was not declared in this scope
../../Source/WebCore/html/parser/HTMLPreloadScanner.cpp: In member function 'void WebCore::TokenPreloadScanner::StartTagScanner::processAttribute(const NameType&, const WTF::String&)':
../../Source/WebCore/html/parser/HTMLPreloadScanner.cpp:154:57: error: 'm_sizesAttribute' was not declared in this scope

Could you check and fix the build?
Comment 9 Yoav Weiss 2015-06-12 05:14:58 PDT
Took also the m_sizesAttribute definition from behind the guard at https://bugs.webkit.org/show_bug.cgi?id=145926

I believe it should do the trick.
Comment 10 Csaba Osztrogonác 2015-06-12 05:17:29 PDT
(In reply to comment #9)
> Took also the m_sizesAttribute definition from behind the guard at
> https://bugs.webkit.org/show_bug.cgi?id=145926
> 
> I believe it should do the trick.

Thanks, I'll check it soon.
Comment 11 zalan 2015-06-12 09:39:08 PDT
Build fix: http://trac.webkit.org/changeset/185501