Bug 173509 – Demote the "we have navigated away" check to an assertion.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

Now that it is ensured that the render tree can't get to the page cache.

[zalan]

Created attachment 313183 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 313183 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Demote the "we have navigated away" check to an assertion.

I don't see anything getting demoted in this patch?

[zalan]

(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 313183 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313183&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Demote the "we have navigated away" check to an assertion.
> 
> I don't see anything getting demoted in this patch?
FrameView* frameView = frame()->document() == this ? frame()->view() : nullptr
vs.
assert() + view()

[Simon Fraser (smfr)]

Comment on attachment 313183 [details]
Patch

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

>>> Source/WebCore/ChangeLog:3
>>> +        Demote the "we have navigated away" check to an assertion.
>> 
>> I don't see anything getting demoted in this patch?
> 
> FrameView* frameView = frame()->document() == this ? frame()->view() : nullptr
> vs.
> assert() + view()

Ah, that's a bit subtle.

[zalan]

(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 313183 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313183&action=review
> 
> >>> Source/WebCore/ChangeLog:3
> >>> +        Demote the "we have navigated away" check to an assertion.
> >> 
> >> I don't see anything getting demoted in this patch?
> > 
> > FrameView* frameView = frame()->document() == this ? frame()->view() : nullptr
> > vs.
> > assert() + view()
> 
> Ah, that's a bit subtle.
yea, but that's the expected behavior now.

[Build Bot]

Comment on attachment 313183 [details]
Patch

Attachment 313183 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3945982

New failing tests:
inspector/canvas/create-canvas-contexts.html

[Build Bot]

Created attachment 313187 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[zalan]

(In reply to Build Bot from comment #6)
> Comment on attachment 313183 [details]
> Patch
> 
> Attachment 313183 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.webkit.org/results/3945982
> 
> New failing tests:
> inspector/canvas/create-canvas-contexts.html

really???

[WebKit Commit Bot]

Comment on attachment 313183 [details]
Patch

Clearing flags on attachment: 313183

Committed r218456: <http://trac.webkit.org/changeset/218456>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 313183 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:2215
> +    ASSERT(frame() && frame()->document() == this);

Please don’t use && in assertions. If you write this as two separate assertions, and half of it failed, we will be able to tell which half.

[zalan]

(In reply to Darin Adler from comment #11)
> Comment on attachment 313183 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313183&action=review
> 
> > Source/WebCore/dom/Document.cpp:2215
> > +    ASSERT(frame() && frame()->document() == this);
> 
> Please don’t use && in assertions. If you write this as two separate
> assertions, and half of it failed, we will be able to tell which half.
Oh, right! I usually keep in mind not to merge them. Not sure what happened here. :( Thanks!

[zalan]

Committed r218462: <http://trac.webkit.org/changeset/218462>