Bug 131117 - Tidy up isIsolatedInline() and highestContainingIsolateWithinRoot()
Summary: Tidy up isIsolatedInline() and highestContainingIsolateWithinRoot()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-02 12:24 PDT by David Kilzer (:ddkilzer)
Modified: 2014-04-17 12:40 PDT (History)
12 users (show)

See Also:


Attachments
Patch v1 (5.70 KB, patch)
2014-04-02 12:28 PDT, David Kilzer (:ddkilzer)
dbates: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (501.01 KB, application/zip)
2014-04-02 13:55 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2014-04-02 12:24:11 PDT
Tidy up isIsolatedInline() and highestContainingIsolateWithinRoot() based on review feedback for r166650.

<http://trac.webkit.org/changeset/166650>
Comment 1 David Kilzer (:ddkilzer) 2014-04-02 12:28:09 PDT
Created attachment 228411 [details]
Patch v1
Comment 2 Build Bot 2014-04-02 13:55:21 PDT
Comment on attachment 228411 [details]
Patch v1

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

New failing tests:
transitions/cancel-transition.html
Comment 3 Build Bot 2014-04-02 13:55:24 PDT
Created attachment 228425 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 4 Daniel Bates 2014-04-07 23:55:19 PDT
Comment on attachment 228411 [details]
Patch v1

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

> Source/WebCore/rendering/InlineIterator.h:466
> +    for (RenderObject* root = iter.root(); object && object != root; object = object->parent()) {

This is OK as-is. Notice that |object| isn't modified outside of the for-loop, InlineIterator::{renderer, root}() are inline functions (since they are defined in the class definition) and that we can take advantage of the for-loop condition to handle the nullity check of |object| instead of performing an explicit nullity check on line 463. We can write the body of this function so as to read:

unsigned count = 0;
for (RenderObject* object = iter.renderer(), root = iter.root(); object && object != root; object = object->parent()) {
    if (isIsolatedInline(*object))
        ++count;
}
return count;
Comment 5 David Kilzer (:ddkilzer) 2014-04-16 16:53:05 PDT
(In reply to comment #4)
> (From update of attachment 228411 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228411&action=review
> 
> > Source/WebCore/rendering/InlineIterator.h:466
> > +    for (RenderObject* root = iter.root(); object && object != root; object = object->parent()) {
> 
> This is OK as-is. Notice that |object| isn't modified outside of the for-loop, InlineIterator::{renderer, root}() are inline functions (since they are defined in the class definition) and that we can take advantage of the for-loop condition to handle the nullity check of |object| instead of performing an explicit nullity check on line 463. We can write the body of this function so as to read:
> 
> unsigned count = 0;
> for (RenderObject* object = iter.renderer(), root = iter.root(); object && object != root; object = object->parent()) {
>     if (isIsolatedInline(*object))
>         ++count;
> }
> return count;

Nice!  Addressing these comments before landing.
Comment 6 David Kilzer (:ddkilzer) 2014-04-16 20:15:11 PDT
(In reply to comment #4)
> for (RenderObject* object = iter.renderer(), root = iter.root(); object && object != root; object = object->parent()) {

This actually doesn't compile.  The reason is that the '*' operator only modifies |object| to be a pointer, while |root| is a RenderObject.

To make this work, we need to use a typedef:

    typedef RenderObject* RenderObjectPtr;
    for (RenderObjectPtr object = iter.renderer(), root = iter.root(); object && object != root; object = object->parent()) {

Is it still worth it?  Is declaring |object| and |root| in the same for() statement with the NULL check clearer than declaring |object| separately before the loop?  (I guess I should look at code gen as well since it's an inline method.)
Comment 7 Daniel Bates 2014-04-16 22:52:53 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > for (RenderObject* object = iter.renderer(), root = iter.root(); object && object != root; object = object->parent()) {
> 
> This actually doesn't compile.  The reason is that the '*' operator only modifies |object| to be a pointer, while |root| is a RenderObject.
> 

You're right!

> To make this work, we need to use a typedef:
> 
>     typedef RenderObject* RenderObjectPtr;

Or declare a temporary variable |root| outside of the for-loop.

>     for (RenderObjectPtr object = iter.renderer(), root = iter.root(); object && object != root; object = object->parent()) {
> 
> Is it still worth it?  Is declaring |object| and |root| in the same for() statement with the NULL check clearer than declaring |object| separately before the loop?  (I guess I should look at code gen as well since it's an inline method.)

My original motivation for suggesting the modification was to remove the extra nullity check and move the initialization of the variable |object| into the for-loop initialization towards making the for-loop more idiomatic. That is, resembling a for-loop for traversing the ancestors of a node. I don't have a strong opinion on such a change. I trust your judgement.
Comment 8 David Kilzer (:ddkilzer) 2014-04-17 12:40:56 PDT
Committed r167447: <http://trac.webkit.org/changeset/167447>