Refactoring: Change the order of conditions to avoid computing rendererIsEditable()

Refactoring: Change the order of conditions to avoid computing rendererIsEditable()

When we need both Node::renderer() and Node::rendererIsEditable()
conditions to be true to perform some operation, it is more effective
to check for renderer() first, so that if this condition fails we can
avoid unnecessary computation of rendererIsEditable().

All the layout tests have been successfully run with this change.

R=ojan, yosin

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158856

[vanihegde, 2013-10-01 10:56:30]

Please have a look at this minor code refactoring.

[ojan, 2013-10-01 19:21:38]

https://codereview.chromium.org/25490002/diff/1/Source/core/dom/Position.cpp
File Source/core/dom/Position.cpp (right):

https://codereview.chromium.org/25490002/diff/1/Source/core/dom/Position.cpp#...
Source/core/dom/Position.cpp:601: RenderObject* renderer =
currentNode->renderer();
Doesn't this change behavior because you've now moved it above the early return?

https://codereview.chromium.org/25490002/diff/1/Source/core/dom/Position.cpp#...
Source/core/dom/Position.cpp:724: RenderObject* renderer =
currentNode->renderer();
Ditto.

[yosin_UTC9, 2013-10-02 01:51:50]

On 2013/10/01 19:21:38, ojan wrote:
> https://codereview.chromium.org/25490002/diff/1/Source/core/dom/Position.cpp
> File Source/core/dom/Position.cpp (right):
> 
>
https://codereview.chromium.org/25490002/diff/1/Source/core/dom/Position.cpp#...
> Source/core/dom/Position.cpp:601: RenderObject* renderer =
> currentNode->renderer();
> Doesn't this change behavior because you've now moved it above the early
return?
> 
>
https://codereview.chromium.org/25490002/diff/1/Source/core/dom/Position.cpp#...
> Source/core/dom/Position.cpp:724: RenderObject* renderer =
> currentNode->renderer();
> Ditto.

As Ojan's comment, we should not move these condition. We don't have confident
to move these conditions even if layout tests passed; coverage of layout test
isn't perfect.

[vanihegde, 2013-10-03 04:13:53]

On 2013/10/02 01:51:50, Yoshifumi Inoue wrote:
> On 2013/10/01 19:21:38, ojan wrote:
> > https://codereview.chromium.org/25490002/diff/1/Source/core/dom/Position.cpp
> > File Source/core/dom/Position.cpp (right):
> > 
> >
>
https://codereview.chromium.org/25490002/diff/1/Source/core/dom/Position.cpp#...
> > Source/core/dom/Position.cpp:601: RenderObject* renderer =
> > currentNode->renderer();
> > Doesn't this change behavior because you've now moved it above the early
> return?
> > 
> >
>
https://codereview.chromium.org/25490002/diff/1/Source/core/dom/Position.cpp#...
> > Source/core/dom/Position.cpp:724: RenderObject* renderer =
> > currentNode->renderer();
> > Ditto.
> 
> As Ojan's comment, we should not move these condition. We don't have confident
> to move these conditions even if layout tests passed; coverage of layout test
> isn't perfect.

Thanks for the quick review ojan and yosin.
I agree, sorry for overlooking the possible behavior change.
But I feel the other changes (expect for upstream and downstream) are still
okay.

Additionally did a minor cleanup - removing unused variable etc. which I had
come
across while going through editing code. Since it's not much, thought of adding
them to the same patch. I hope that's fine.

[yosin_UTC9, 2013-10-03 09:56:31]

> Additionally did a minor cleanup - removing unused variable etc. which I had
> come
> across while going through editing code. Since it's not much, thought of
adding
> them to the same patch. I hope that's fine.

Please put changes not related to "Change the order of conditions to avoid
computing rendererIsEditable()" into another patch for
 - Make this patch what to do - for eace of tracking, e.g. see blame/annotate
view in next year
 - Minimize redoing after roll out - I know this patch won't roll out, but it is
in case.

Unrelated changes sometime make people confusing especially for investigating
root cause of bug.

Thanks for your understanding and cooperation.
-yosi

[vanihegde, 2013-10-03 09:59:55]

On 2013/10/03 09:56:31, Yoshifumi Inoue wrote:
> > Additionally did a minor cleanup - removing unused variable etc. which I had
> > come
> > across while going through editing code. Since it's not much, thought of
> adding
> > them to the same patch. I hope that's fine.
> 
> Please put changes not related to "Change the order of conditions to avoid
> computing rendererIsEditable()" into another patch for
>  - Make this patch what to do - for eace of tracking, e.g. see blame/annotate
> view in next year
>  - Minimize redoing after roll out - I know this patch won't roll out, but it
is
> in case.
> 
> Unrelated changes sometime make people confusing especially for investigating
> root cause of bug.
> 
> Thanks for your understanding and cooperation.
> -yosi

Sure, I shall do that right away.

[vanihegde, 2013-10-03 10:07:37]

Knocked off changes not related. Please have a look. Sorry for the trouble.

[ojan, 2013-10-03 19:59:27]

lgtm

[commit-bot: I haz the power, 2013-10-03 20:02:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vani.hegde@samsung.com/25490002/12001

[commit-bot: I haz the power, 2013-10-04 01:25:09]

Change committed as 158856