Bug 140056 - Modernize and streamline HTMLStackItem
Summary: Modernize and streamline HTMLStackItem
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-04 12:06 PST by Darin Adler
Modified: 2015-01-05 11:23 PST (History)
6 users (show)

See Also:

Attachments
Patch (120.54 KB, patch)
2015-01-04 12:16 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mountainlion (263.53 KB, application/zip)
2015-01-04 12:35 PST, Build Bot 		no flags Details
Archive of layout-test-results from ews105 for mac-mountainlion-wk2 (254.66 KB, application/zip)
2015-01-04 12:48 PST, Build Bot 		no flags Details
Patch (120.51 KB, patch)
2015-01-04 16:21 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (120.69 KB, patch)
2015-01-05 00:37 PST, Darin Adler 		andersca: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2015-01-04 12:06:08 PST 
Modernize and streamline HTMLStackItem
Comment 1 Darin Adler 2015-01-04 12:16:23 PST 
Created attachment 243936 [details]
Patch
Comment 2 Build Bot 2015-01-04 12:35:29 PST 
Comment on attachment 243936 [details]
Patch

Attachment 243936 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5627747011395584

Number of test failures exceeded the failure limit.
Comment 3 Build Bot 2015-01-04 12:35:32 PST 
Created attachment 243938 [details]
Archive of layout-test-results from ews102 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 4 Build Bot 2015-01-04 12:48:23 PST 
Comment on attachment 243936 [details]
Patch

Attachment 243936 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4856332632981504

Number of test failures exceeded the failure limit.
Comment 5 Build Bot 2015-01-04 12:48:26 PST 
Created attachment 243940 [details]
Archive of layout-test-results from ews105 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 6 Darin Adler 2015-01-04 16:21:58 PST 
Created attachment 243948 [details]
Patch
Comment 7 Darin Adler 2015-01-05 00:37:50 PST 
Created attachment 243967 [details]
Patch
Comment 8 Anders Carlsson 2015-01-05 08:24:18 PST 
Comment on attachment 243967 [details]
Patch

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

> Source/WebCore/html/parser/HTMLStackItem.h:3
> + * Copyright (C) 2015 Apple Inc. All rights reserved.

Nice.

> Source/WebCore/html/parser/HTMLStackItem.h:42
> +    static Ref<HTMLStackItem> create(Ref<Element>, AtomicHTMLToken&, const AtomicString& namespaceURI = HTMLNames::xhtmlNamespaceURI);

Shouldn't Ref<Element> be Ref<Element>&& here?

> Source/WebCore/html/parser/HTMLStackItem.h:64
> +    HTMLStackItem(Ref<Element>, AtomicHTMLToken&, const AtomicString& namespaceURI);

And here.
Comment 9 Darin Adler 2015-01-05 09:11:39 PST 
Committed r177910: <http://trac.webkit.org/changeset/177910>
Comment 10 Darin Adler 2015-01-05 09:26:40 PST 
Comment on attachment 243967 [details]
Patch

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

>> Source/WebCore/html/parser/HTMLStackItem.h:42
>> +    static Ref<HTMLStackItem> create(Ref<Element>, AtomicHTMLToken&, const AtomicString& namespaceURI = HTMLNames::xhtmlNamespaceURI);
> 
> Shouldn't Ref<Element> be Ref<Element>&& here?

I guess so. Sorry missed that, so I’ll do it in a follow up.

>> Source/WebCore/html/parser/HTMLStackItem.h:64
>> +    HTMLStackItem(Ref<Element>, AtomicHTMLToken&, const AtomicString& namespaceURI);
> 
> And here.

Will do.
Comment 11 David Kilzer (:ddkilzer) 2015-01-05 10:15:26 PST 
(In reply to comment #9)
> Committed r177910: <http://trac.webkit.org/changeset/177910>

This seems to have broken trunk iOS builds as well:

Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2525:111: error: indirection requires pointer operand ('WebCore::ContainerNode' invalid)
    if (!isParsingFragment() && m_tree.isTelephoneNumberParsingEnabled() && shouldParseTelephoneNumbersInNode(*m_tree.currentNode()) && TelephoneNumberDetector::isSupported())
                                                                                                              ^~~~~~~~~~~~~~~~~~~~~
1 error generated.
Comment 12 David Kilzer (:ddkilzer) 2015-01-05 11:23:41 PST 
(In reply to comment #11)
> (In reply to comment #9)
> > Committed r177910: <http://trac.webkit.org/changeset/177910>
> 
> This seems to have broken trunk iOS builds as well:
> 
> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2525:111: error: indirection
> requires pointer operand ('WebCore::ContainerNode' invalid)
>     if (!isParsingFragment() && m_tree.isTelephoneNumberParsingEnabled() &&
> shouldParseTelephoneNumbersInNode(*m_tree.currentNode()) &&
> TelephoneNumberDetector::isSupported())
>                                                                             
> ^~~~~~~~~~~~~~~~~~~~~
> 1 error generated.

Attempted fix by Dan Bernstein:  http://trac.webkit.org/changeset/177916