Refactoring out the code in LayoutTreeAsText::writeLayoutObject.

Refactoring out the code in LayoutTreeAsText::writeLayoutObject.

BUG=481588
Committed: https://crrev.com/ecf51ddf5158be270b3ad65d613c9498d644fc6b
Cr-Commit-Position: refs/heads/master@{#419072}

[lunalu1, 2016-08-26 20:16:36]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[lunalu1, 2016-08-26 20:16:41]

The CQ bit was unchecked by lunalu@chromium.org

[lunalu1, 2016-08-26 20:17:02]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 20:18:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 22:01:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 22:01:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lunalu1, 2016-08-29 14:17:37]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 14:18:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-08-29 14:44:03]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 14:44:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 14:50:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 14:50:43]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lunalu1, 2016-08-29 14:55:02]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 14:55:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-08-29 15:41:17]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 15:41:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 16:29:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 16:29:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lunalu1, 2016-08-29 17:40:43]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 17:50:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-08-29 19:10:21]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 19:47:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[benjhayden, 2016-08-29 20:45:26]

benjhayden@chromium.org changed reviewers:
+ benjhayden@chromium.org

[benjhayden, 2016-08-29 20:45:28]

https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4876: if (cb &&
cb->isTableCell())
Should this be moved to a virtual method so that you don't need to check
isTableCell and then call toLayoutTableCell?

https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1372: // FIXME: Would be
better not to just dump 0, 0 as the x and y here.
Would it be possible to go ahead and use linesBox.x() and linesBox.y() in this
CL so as not to break TracedLayoutObject? I realize that might mean updating a
bunch of layout tests, but perhaps layout-dev has a feasible way to do that?

https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3206: if (isLayoutView())
{
Should this be moved to a virtual method so that LayoutView can override it?

https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp (left):

https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp:40:
tracedValue->setDouble("absX", object.absoluteBoundingBoxRect().x());
Can you restore absX and absY? Sorry, I realize I accidentally asked you to
delete them, my bad. :-)

[commit-bot: I haz the power, 2016-08-30 03:35:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 03:35:44]

Dry run: This issue passed the CQ dry run.

[lunalu1, 2016-08-30 20:35:16]

On 2016/08/29 20:45:28, benjhayden wrote:
>
https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):
> 
>
https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:4876: if (cb &&
> cb->isTableCell())
> Should this be moved to a virtual method so that you don't need to check
> isTableCell and then call toLayoutTableCell?
> 
>
https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):
> 
>
https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutInline.cpp:1372: // FIXME: Would
be
> better not to just dump 0, 0 as the x and y here.
> Would it be possible to go ahead and use linesBox.x() and linesBox.y() in this
> CL so as not to break TracedLayoutObject? I realize that might mean updating a
> bunch of layout tests, but perhaps layout-dev has a feasible way to do that?
> 
>
https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutObject.cpp:3206: if
(isLayoutView())
> {
> Should this be moved to a virtual method so that LayoutView can override it?
> 
>
https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp (left):
> 
>
https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp:40:
> tracedValue->setDouble("absX", object.absoluteBoundingBoxRect().x());
> Can you restore absX and absY? Sorry, I realize I accidentally asked you to
> delete them, my bad. :-)

Hi Ben,

I made some changes based on your comments. I am not so confident with some of
the changes, could you please take a look?

I didn't make the change in LayoutObject.cpp:3206 as you requested because I
wasn't so clear about what you were suggesting. Could you please clarify on
that.

Thank you for your time,
Luna

[lunalu1, 2016-08-30 20:35:30]

https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4876: if (cb &&
cb->isTableCell())
On 2016/08/29 20:45:27, benjhayden wrote:
> Should this be moved to a virtual method so that you don't need to check
> isTableCell and then call toLayoutTableCell? 

I moved this part to a virtual method but I don't think I did it right. Could
you please be more clear about how to make this part virtual?

Thanks

https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1372: // FIXME: Would be
better not to just dump 0, 0 as the x and y here.
On 2016/08/29 20:45:27, benjhayden wrote:
> Would it be possible to go ahead and use linesBox.x() and linesBox.y() in this
> CL so as not to break TracedLayoutObject? I realize that might mean updating a
> bunch of layout tests, but perhaps layout-dev has a feasible way to do that?

I will use linesBox.x() and linesBox.y() for now, and figure out which layout
tests to be updated later.

https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3206: if (isLayoutView())
{
On 2016/08/29 20:45:27, benjhayden wrote:
> Should this be moved to a virtual method so that LayoutView can override it?

Could you please clarify on this? So to create a virtual method that essentially
do nothing, but LayoutView override it to update the width and height? 

Can a LayoutObject be both a LayoutText/LayoutInline/LayoutTableCell/LayoutBox
and a LayoutView?

https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp (left):

https://codereview.chromium.org/2280153002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp:40:
tracedValue->setDouble("absX", object.absoluteBoundingBoxRect().x());
On 2016/08/29 20:45:27, benjhayden wrote:
> Can you restore absX and absY? Sorry, I realize I accidentally asked you to
> delete them, my bad. :-)

Done.

[blink-reviews, 2016-09-02 20:04:50]

Hi Ben,

When you have time, do you mind to reply to my comments?

Thanks


Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Mon, Aug 29, 2016 at 4:45 PM, <benjhayden@chromium.org> wrote:

>
> https://codereview.chromium.org/2280153002/diff/120001/
> third_party/WebKit/Source/core/layout/LayoutBox.cpp
> File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):
>
> https://codereview.chromium.org/2280153002/diff/120001/
> third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode4876
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:4876: if (cb &&
> cb->isTableCell())
> Should this be moved to a virtual method so that you don't need to check
> isTableCell and then call toLayoutTableCell?
>
> https://codereview.chromium.org/2280153002/diff/120001/
> third_party/WebKit/Source/core/layout/LayoutInline.cpp
> File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):
>
> https://codereview.chromium.org/2280153002/diff/120001/
> third_party/WebKit/Source/core/layout/LayoutInline.cpp#newcode1372
> third_party/WebKit/Source/core/layout/LayoutInline.cpp:1372: // FIXME:
> Would be better not to just dump 0, 0 as the x and y here.
> Would it be possible to go ahead and use linesBox.x() and linesBox.y()
> in this CL so as not to break TracedLayoutObject? I realize that might
> mean updating a bunch of layout tests, but perhaps layout-dev has a
> feasible way to do that?
>
> https://codereview.chromium.org/2280153002/diff/120001/
> third_party/WebKit/Source/core/layout/LayoutObject.cpp
> File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
>
> https://codereview.chromium.org/2280153002/diff/120001/
> third_party/WebKit/Source/core/layout/LayoutObject.cpp#newcode3206
> third_party/WebKit/Source/core/layout/LayoutObject.cpp:3206: if
> (isLayoutView()) {
> Should this be moved to a virtual method so that LayoutView can override
> it?
>
> https://codereview.chromium.org/2280153002/diff/120001/
> third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp
> File third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp
> (left):
>
> https://codereview.chromium.org/2280153002/diff/120001/
> third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp#oldcode40
> third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp:40:
> tracedValue->setDouble("absX", object.absoluteBoundingBoxRect().x());
> Can you restore absX and absY? Sorry, I realize I accidentally asked you
> to delete them, my bad. :-)
>
> https://codereview.chromium.org/2280153002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-09-02 20:04:51]

Hi Ben,

When you have time, do you mind to reply to my comments?

Thanks


Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Mon, Aug 29, 2016 at 4:45 PM, <benjhayden@chromium.org> wrote:

>
> https://codereview.chromium.org/2280153002/diff/120001/
> third_party/WebKit/Source/core/layout/LayoutBox.cpp
> File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):
>
> https://codereview.chromium.org/2280153002/diff/120001/
> third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode4876
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:4876: if (cb &&
> cb->isTableCell())
> Should this be moved to a virtual method so that you don't need to check
> isTableCell and then call toLayoutTableCell?
>
> https://codereview.chromium.org/2280153002/diff/120001/
> third_party/WebKit/Source/core/layout/LayoutInline.cpp
> File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):
>
> https://codereview.chromium.org/2280153002/diff/120001/
> third_party/WebKit/Source/core/layout/LayoutInline.cpp#newcode1372
> third_party/WebKit/Source/core/layout/LayoutInline.cpp:1372: // FIXME:
> Would be better not to just dump 0, 0 as the x and y here.
> Would it be possible to go ahead and use linesBox.x() and linesBox.y()
> in this CL so as not to break TracedLayoutObject? I realize that might
> mean updating a bunch of layout tests, but perhaps layout-dev has a
> feasible way to do that?
>
> https://codereview.chromium.org/2280153002/diff/120001/
> third_party/WebKit/Source/core/layout/LayoutObject.cpp
> File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
>
> https://codereview.chromium.org/2280153002/diff/120001/
> third_party/WebKit/Source/core/layout/LayoutObject.cpp#newcode3206
> third_party/WebKit/Source/core/layout/LayoutObject.cpp:3206: if
> (isLayoutView()) {
> Should this be moved to a virtual method so that LayoutView can override
> it?
>
> https://codereview.chromium.org/2280153002/diff/120001/
> third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp
> File third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp
> (left):
>
> https://codereview.chromium.org/2280153002/diff/120001/
> third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp#oldcode40
> third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp:40:
> tracedValue->setDouble("absX", object.absoluteBoundingBoxRect().x());
> Can you restore absX and absY? Sorry, I realize I accidentally asked you
> to delete them, my bad. :-)
>
> https://codereview.chromium.org/2280153002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[lunalu1, 2016-09-06 19:03:37]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 19:04:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 19:07:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 19:07:03]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[lunalu1, 2016-09-06 19:19:19]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 19:20:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-09-06 19:53:52]

Hi Ben, 

Thank you for the feedback.

I have updated the CL based on your comments. Please take another look.

Regards,
Luna

[commit-bot: I haz the power, 2016-09-06 21:08:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 21:08:48]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lunalu1, 2016-09-07 17:57:15]

Patchset #1 (id:1) has been deleted

[lunalu1, 2016-09-07 17:57:22]

Patchset #1 (id:20001) has been deleted

[lunalu1, 2016-09-07 17:57:34]

Patchset #1 (id:40001) has been deleted

[lunalu1, 2016-09-07 17:57:46]

Patchset #3 (id:100001) has been deleted

[lunalu1, 2016-09-07 17:58:05]

Patchset #1 (id:60001) has been deleted

[lunalu1, 2016-09-07 17:58:13]

Patchset #3 (id:140001) has been deleted

[lunalu1, 2016-09-07 17:58:21]

Patchset #3 (id:160001) has been deleted

[lunalu1, 2016-09-07 17:58:32]

Patchset #1 (id:80001) has been deleted

[benjhayden, 2016-09-08 20:18:07]

This is probably my last comment. I'll try to see if I can get somebody on
layout-dev to take a look.

https://codereview.chromium.org/2280153002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlock.h (right):

https://codereview.chromium.org/2280153002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlock.h:431: virtual void
adjustForTableCells(LayoutRect&) const {}
Can you rename this to adjustChildDebugRect()?
I appreciate keeping the variable name to make review easier, but as a method
name it's unclear.

[lunalu1, 2016-09-09 15:08:32]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 15:08:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-09-09 15:09:43]

Renamed the helper as suggested. Please take another look. Thanks

https://codereview.chromium.org/2280153002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlock.h (right):

https://codereview.chromium.org/2280153002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlock.h:431: virtual void
adjustForTableCells(LayoutRect&) const {}
On 2016/09/08 20:18:07, benjhayden wrote:
> Can you rename this to adjustChildDebugRect()?
> I appreciate keeping the variable name to make review easier, but as a method
> name it's unclear.

Done.

[commit-bot: I haz the power, 2016-09-09 16:55:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 16:55:39]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[benjhayden, 2016-09-09 18:04:18]

benjhayden@chromium.org changed reviewers:
+ skobes@chromium.org

[benjhayden, 2016-09-09 18:04:19]

Luna, Steve has agreed to review this.

Thanks for reviewing this, Steve!

[benjhayden, 2016-09-09 18:04:53]

lgtm, please wait for skobes

[skobes, 2016-09-09 20:27:41]

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlock.h (right):

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlock.h:429: // FIXME: Temporary in
order ot ensure compatibility with existing layout test
Current Blink style is to use "TODO(username)" instead of FIXME.

Also, typo: "ot" -> "to"

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4856: LayoutRect r =
frameRect();
We generally avoid single-letter abbreviations in variable names.  Can you call
this "rect" (and "block" for the containing block below)?

Same in the other files that do this.

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1372: // FIXME: Would be
better not to just dump 0, 0 as the x and y here.
It looks like this FIXME has been addressed?

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1375: IntRect linesBox =
enclosingIntRect(linesBoundingBox());
Why aren't we just returning linesBoundingBox()?  This will lose subpixel
precision.

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1376: LayoutRect r =
LayoutRect(IntRect(IntRect(linesBox.x(), linesBox.y(), linesBox.width(),
linesBox.height())));
Remove redundant cast to IntRect.

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:82: #include
"platform/scroll/ScrollableArea.h"
Do you need this?  There are no other changes in the file.

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1369: // Fixed
implementation for LayoutText, LayoutInline, LayoutTableCell and
The comment should describe what the method does, e.g. "Returns a rect
corresponding to this LayoutObject's bounds for use in debugging output".  You
don't need to list the classes that override it.

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1373: // This method should
never be reached as it's overriden by subclasses.
Why not just make it pure virtual?  Then the compiler will catch any missing
overrides.

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1050: LayoutRect r =
LayoutRect(location().x(), location().y() + intrinsicPaddingBefore(),
size().width(), size().height() - intrinsicPaddingBefore() -
intrinsicPaddingAfter());
Wrap this line at the commas for readability.

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1771: IntRect linesBox =
enclosingIntRect(linesBoundingBox());
Same question re. rounding to ints.

[lunalu1, 2016-09-12 14:43:56]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 14:44:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-09-12 14:45:14]

Thank you for reviewing my CL. 
I have made the change based on your comments (I am not so sure if I did it
correctly upon change in LayoutText.cpp). Please take another look. 

Regards,
Luna

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlock.h (right):

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlock.h:429: // FIXME: Temporary in
order ot ensure compatibility with existing layout test
On 2016/09/09 20:27:41, skobes wrote:
> Current Blink style is to use "TODO(username)" instead of FIXME.
> 
> Also, typo: "ot" -> "to"

Done.

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4856: LayoutRect r =
frameRect();
On 2016/09/09 20:27:41, skobes wrote:
> We generally avoid single-letter abbreviations in variable names.  Can you
call
> this "rect" (and "block" for the containing block below)?
> 
> Same in the other files that do this.

Done.

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1372: // FIXME: Would be
better not to just dump 0, 0 as the x and y here.
On 2016/09/09 20:27:41, skobes wrote:
> It looks like this FIXME has been addressed?

Done.

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1375: IntRect linesBox =
enclosingIntRect(linesBoundingBox());
On 2016/09/09 20:27:41, skobes wrote:
> Why aren't we just returning linesBoundingBox()?  This will lose subpixel
> precision.

Done.

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1376: LayoutRect r =
LayoutRect(IntRect(IntRect(linesBox.x(), linesBox.y(), linesBox.width(),
linesBox.height())));
On 2016/09/09 20:27:41, skobes wrote:
> Remove redundant cast to IntRect.

Done.

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:82: #include
"platform/scroll/ScrollableArea.h"
On 2016/09/09 20:27:41, skobes wrote:
> Do you need this?  There are no other changes in the file.

Done.

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1369: // Fixed
implementation for LayoutText, LayoutInline, LayoutTableCell and
On 2016/09/09 20:27:41, skobes wrote:
> The comment should describe what the method does, e.g. "Returns a rect
> corresponding to this LayoutObject's bounds for use in debugging output".  You
> don't need to list the classes that override it.

Done.

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1373: // This method should
never be reached as it's overriden by subclasses.
On 2016/09/09 20:27:41, skobes wrote:
> Why not just make it pure virtual?  Then the compiler will catch any missing
> overrides.

Then I have to write an overriding function for subclasses:
LayoutSVGGradientStop and LayoutSVGImage

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1050: LayoutRect r =
LayoutRect(location().x(), location().y() + intrinsicPaddingBefore(),
size().width(), size().height() - intrinsicPaddingBefore() -
intrinsicPaddingAfter());
On 2016/09/09 20:27:41, skobes wrote:
> Wrap this line at the commas for readability.

Done.

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1771: IntRect linesBox =
enclosingIntRect(linesBoundingBox());
On 2016/09/09 20:27:41, skobes wrote:
> Same question re. rounding to ints.

Done.

[skobes, 2016-09-12 17:09:35]

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2280153002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1373: // This method should
never be reached as it's overriden by subclasses.
On 2016/09/12 14:45:14, loonybear wrote:
> On 2016/09/09 20:27:41, skobes wrote:
> > Why not just make it pure virtual?  Then the compiler will catch any missing
> > overrides.
> 
> Then I have to write an overriding function for subclasses:
> LayoutSVGGradientStop and LayoutSVGImage

If there are subclasses that don't override it, then why does it call
NOTREACHED()?

https://codereview.chromium.org/2280153002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/2280153002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1375: LayoutRect rect =
LayoutRect(linesBox);
This line doesn't do anything.  You can inline the call to linesBoundingBox in
the return statement, like this:

  return linesBoundingBox();

https://codereview.chromium.org/2280153002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2280153002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutView.cpp:1006:
block->adjustChildDebugRect(rect);
At this point, the rect is empty.  Should this be done after the width and
height are set?

[commit-bot: I haz the power, 2016-09-12 17:16:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 17:16:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lunalu1, 2016-09-12 17:48:48]

https://codereview.chromium.org/2280153002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/2280153002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1375: LayoutRect rect =
LayoutRect(linesBox);
On 2016/09/12 17:09:35, skobes wrote:
> This line doesn't do anything.  You can inline the call to linesBoundingBox in
> the return statement, like this:
> 
>   return linesBoundingBox();

Done.

https://codereview.chromium.org/2280153002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2280153002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutView.cpp:1006:
block->adjustChildDebugRect(rect);
On 2016/09/12 17:09:35, skobes wrote:
> At this point, the rect is empty.  Should this be done after the width and
> height are set?

I did this part according to the following code in LayoutTreeAsText.cpp:186-221.
And it seems like the adjustChildDebugRect() (as adjustForTableCells) was done
before setting the width and height for the LayoutView object.

    LayoutBlock* cb = o.containingBlock();
    bool adjustForTableCells = cb ? cb->isTableCell() : false;

    LayoutRect r;
    if (o.isText()) {
        ...
    } else if (o.isLayoutInline()) {
        ...
    } else if (o.isTableCell()) {
       ...
    } else if (o.isBox()) {
       ...
    }

    // FIXME: Temporary in order to ensure compatibility with existing layout
test results.
    if (adjustForTableCells)
        r.move(0,
-toLayoutTableCell(o.containingBlock())->intrinsicPaddingBefore());

    if (o.isLayoutView()) {
        r.setWidth(LayoutUnit(toLayoutView(o).viewWidth(IncludeScrollbars)));
        r.setHeight(LayoutUnit(toLayoutView(o).viewHeight(IncludeScrollbars)));
    }

[skobes, 2016-09-12 17:54:05]

https://codereview.chromium.org/2280153002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2280153002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutView.cpp:1006:
block->adjustChildDebugRect(rect);
On 2016/09/12 17:48:48, loonybear wrote:
> On 2016/09/12 17:09:35, skobes wrote:
> > At this point, the rect is empty.  Should this be done after the width and
> > height are set?
> 
> I did this part according to the following code in
LayoutTreeAsText.cpp:186-221.
> And it seems like the adjustChildDebugRect() (as adjustForTableCells) was done
> before setting the width and height for the LayoutView object.

I see.  I guess the order doesn't matter since adjustChildDebugRect doesn't care
about the size (and is temporary anyway), so this is ok.

[lunalu1, 2016-09-12 18:26:48]

https://codereview.chromium.org/2280153002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2280153002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutView.cpp:1006:
block->adjustChildDebugRect(rect);
On 2016/09/12 17:54:05, skobes wrote:
> On 2016/09/12 17:48:48, loonybear wrote:
> > On 2016/09/12 17:09:35, skobes wrote:
> > > At this point, the rect is empty.  Should this be done after the width and
> > > height are set?
> > 
> > I did this part according to the following code in
> LayoutTreeAsText.cpp:186-221.
> > And it seems like the adjustChildDebugRect() (as adjustForTableCells) was
done
> > before setting the width and height for the LayoutView object.
> 
> I see.  I guess the order doesn't matter since adjustChildDebugRect doesn't
care
> about the size (and is temporary anyway), so this is ok.

Done.

[skobes, 2016-09-12 18:29:40]

My only remaining concern is the one about making debugRect pure virtual.

[lunalu1, 2016-09-12 18:38:42]

On 2016/09/12 18:29:40, skobes wrote:
> My only remaining concern is the one about making debugRect pure virtual.

So should I make it pure virtual and for the three other inherent classes, just
write the method as {LayoutRect rect; return rect;} ?

[skobes, 2016-09-12 18:43:36]

On 2016/09/12 18:38:42, loonybear wrote:
> So should I make it pure virtual and for the three other inherent classes,
just
> write the method as {LayoutRect rect; return rect;} ?

Ideally they would have a more meaningful implementation.  What does
LayoutTreeAsText currently do when it encounters instances of these classes?

[lunalu1, 2016-09-12 19:25:30]

On 2016/09/12 18:43:36, skobes wrote:
> On 2016/09/12 18:38:42, loonybear wrote:
> > So should I make it pure virtual and for the three other inherent classes,
> just
> > write the method as {LayoutRect rect; return rect;} ?
> 
> Ideally they would have a more meaningful implementation.  What does
> LayoutTreeAsText currently do when it encounters instances of these classes?

Thanks for pointing that out (my previous implementation was wrong). I
misinterpreted the code before.

For the other inherent classes, besides adjustForTableCells (if the containing
block is TableCell), it just creates a rect. Please see the code below.

I guess I could either:
A. define debugRect() in LayoutObject.cpp as:

    LayoutRect rect;
    LayoutBlock* block = containingBlock();
    if (block)
        block->adjustChildDebugRect(rect);
    return rect;

Or
B. make debugRect() pure virtual and define it explicitly for the other inherent
classes (in this case, there will be duplication). 

So I think the option A makes sense.


Here is the current code in LayoutTreeAsText:

    LayoutBlock* cb = o.containingBlock();
    bool adjustForTableCells = cb ? cb->isTableCell() : false;

    LayoutRect r;
    if (o.isText()) {
        ...
    } else if (o.isLayoutInline()) {
        ...
    } else if (o.isTableCell()) {
        ...
    } else if (o.isBox()) {
        ...
    }

    // FIXME: Temporary in order to ensure compatibility with existing layout
test results.
    if (adjustForTableCells)
        r.move(0,
-toLayoutTableCell(o.containingBlock())->intrinsicPaddingBefore());

    if (o.isLayoutView()) {
        ...
    }

    ts << " " << r;

[lunalu1, 2016-09-12 19:25:47]

Please see the updated change.

[skobes, 2016-09-12 20:20:28]

lgtm

[lunalu1, 2016-09-13 14:10:47]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 14:11:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 14:13:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 14:13:52]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lunalu1, 2016-09-13 18:17:24]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 18:18:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 20:14:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 20:14:11]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[blink-reviews, 2016-09-14 14:50:03]

The problem comes from LayoutText.cpp. The original code looks like:
 LayoutRect LayoutText::debugRect() const
 {
     IntRect linesBox = enclosingIntRect(linesBoundingBox());
     LayoutRect rect = LayoutRect(IntRect(firstRunX(), firstRunY(),
linesBox.width(), linesBox.height()));
     ...    return rect;
 }

The current code looks like:

 LayoutRect LayoutText::debugRect() const
 {
     LayoutRect linesBox = linesBoundingBox();
     LayoutRect rect = LayoutRect(IntRect(firstRunX(), firstRunY(),
linesBox.width().toInt(), linesBox.height().toInt()));
     ...    return rect;
 }


​I am not sure if we should update the layout tests or revert the change.​

​Regards,​


Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Mon, Sep 12, 2016 at 2:43 PM, <skobes@chromium.org> wrote:

> On 2016/09/12 18:38:42, loonybear wrote:
> > So should I make it pure virtual and for the three other inherent
> classes,
> just
> > write the method as {LayoutRect rect; return rect;} ?
>
> Ideally they would have a more meaningful implementation. What does
> LayoutTreeAsText currently do when it encounters instances of these
> classes?
>
> https://codereview.chromium.org/2280153002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-09-14 14:50:04]

The problem comes from LayoutText.cpp. The original code looks like:
 LayoutRect LayoutText::debugRect() const
 {
     IntRect linesBox = enclosingIntRect(linesBoundingBox());
     LayoutRect rect = LayoutRect(IntRect(firstRunX(), firstRunY(),
linesBox.width(), linesBox.height()));
     ...    return rect;
 }

The current code looks like:

 LayoutRect LayoutText::debugRect() const
 {
     LayoutRect linesBox = linesBoundingBox();
     LayoutRect rect = LayoutRect(IntRect(firstRunX(), firstRunY(),
linesBox.width().toInt(), linesBox.height().toInt()));
     ...    return rect;
 }


​I am not sure if we should update the layout tests or revert the change.​

​Regards,​


Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Mon, Sep 12, 2016 at 2:43 PM, <skobes@chromium.org> wrote:

> On 2016/09/12 18:38:42, loonybear wrote:
> > So should I make it pure virtual and for the three other inherent
> classes,
> just
> > write the method as {LayoutRect rect; return rect;} ?
>
> Ideally they would have a more meaningful implementation. What does
> LayoutTreeAsText currently do when it encounters instances of these
> classes?
>
> https://codereview.chromium.org/2280153002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[skobes, 2016-09-14 16:35:23]

On 2016/09/14 14:50:04, chromium-reviews wrote:
> The problem comes from LayoutText.cpp. The original code looks like:
>  LayoutRect LayoutText::debugRect() const
>  {
>      IntRect linesBox = enclosingIntRect(linesBoundingBox());
>      LayoutRect rect = LayoutRect(IntRect(firstRunX(), firstRunY(),
> linesBox.width(), linesBox.height()));
>      ...    return rect;
>  }
> 
> The current code looks like:
> 
>  LayoutRect LayoutText::debugRect() const
>  {
>      LayoutRect linesBox = linesBoundingBox();
>      LayoutRect rect = LayoutRect(IntRect(firstRunX(), firstRunY(),
> linesBox.width().toInt(), linesBox.height().toInt()));
>      ...    return rect;
>  }

Neither of these seem ideal.  Can we keep everything in LayoutUnits?

[blink-reviews, 2016-09-14 17:46:17]

Now that I change it to
    LayoutRect LayoutText::debugRect() const
    {
        LayoutRect linesBox = linesBoundingBox();
        LayoutRect rect =
LayoutRect(enclosingIntRect(LayoutRect(LayoutUnit(firstRunX()),
LayoutUnit(firstRunY()), LayoutUnit(linesBox.width()),
LayoutUnit(linesBox.height()))));
        ...
        return rect;
    }

The test results match the expectation now. But I am not so convinced this
is the right fix. Is there anything cleaner I can do?

Thanks


Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Wed, Sep 14, 2016 at 12:35 PM, <skobes@chromium.org> wrote:

> On 2016/09/14 14:50:04, chromium-reviews wrote:
> > The problem comes from LayoutText.cpp. The original code looks like:
> > LayoutRect LayoutText::debugRect() const
> > {
> > IntRect linesBox = enclosingIntRect(linesBoundingBox());
> > LayoutRect rect = LayoutRect(IntRect(firstRunX(), firstRunY(),
> > linesBox.width(), linesBox.height()));
> > ... return rect;
> > }
> >
> > The current code looks like:
> >
> > LayoutRect LayoutText::debugRect() const
> > {
> > LayoutRect linesBox = linesBoundingBox();
> > LayoutRect rect = LayoutRect(IntRect(firstRunX(), firstRunY(),
> > linesBox.width().toInt(), linesBox.height().toInt()));
> > ... return rect;
> > }
>
> Neither of these seem ideal. Can we keep everything in LayoutUnits?
>
> https://codereview.chromium.org/2280153002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-09-14 17:46:18]

Now that I change it to
    LayoutRect LayoutText::debugRect() const
    {
        LayoutRect linesBox = linesBoundingBox();
        LayoutRect rect =
LayoutRect(enclosingIntRect(LayoutRect(LayoutUnit(firstRunX()),
LayoutUnit(firstRunY()), LayoutUnit(linesBox.width()),
LayoutUnit(linesBox.height()))));
        ...
        return rect;
    }

The test results match the expectation now. But I am not so convinced this
is the right fix. Is there anything cleaner I can do?

Thanks


Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Wed, Sep 14, 2016 at 12:35 PM, <skobes@chromium.org> wrote:

> On 2016/09/14 14:50:04, chromium-reviews wrote:
> > The problem comes from LayoutText.cpp. The original code looks like:
> > LayoutRect LayoutText::debugRect() const
> > {
> > IntRect linesBox = enclosingIntRect(linesBoundingBox());
> > LayoutRect rect = LayoutRect(IntRect(firstRunX(), firstRunY(),
> > linesBox.width(), linesBox.height()));
> > ... return rect;
> > }
> >
> > The current code looks like:
> >
> > LayoutRect LayoutText::debugRect() const
> > {
> > LayoutRect linesBox = linesBoundingBox();
> > LayoutRect rect = LayoutRect(IntRect(firstRunX(), firstRunY(),
> > linesBox.width().toInt(), linesBox.height().toInt()));
> > ... return rect;
> > }
>
> Neither of these seem ideal. Can we keep everything in LayoutUnits?
>
> https://codereview.chromium.org/2280153002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blink-reviews, 2016-09-14 17:57:33]

May I ask why we want the expected results to be all INT values?


Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Wed, Sep 14, 2016 at 1:44 PM, Luna Lu <lunalu@google.com> wrote:

> Now that I change it to
>     LayoutRect LayoutText::debugRect() const
>     {
>         LayoutRect linesBox = linesBoundingBox();
>         LayoutRect rect = LayoutRect(enclosingIntRect(
> LayoutRect(LayoutUnit(firstRunX()), LayoutUnit(firstRunY()),
> LayoutUnit(linesBox.width()), LayoutUnit(linesBox.height()))));
>         ...
>         return rect;
>     }
>
> The test results match the expectation now. But I am not so convinced this
> is the right fix. Is there anything cleaner I can do?
>
> Thanks
>
>
> Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751
>
> On Wed, Sep 14, 2016 at 12:35 PM, <skobes@chromium.org> wrote:
>
>> On 2016/09/14 14:50:04, chromium-reviews wrote:
>> > The problem comes from LayoutText.cpp. The original code looks like:
>> > LayoutRect LayoutText::debugRect() const
>> > {
>> > IntRect linesBox = enclosingIntRect(linesBoundingBox());
>> > LayoutRect rect = LayoutRect(IntRect(firstRunX(), firstRunY(),
>> > linesBox.width(), linesBox.height()));
>> > ... return rect;
>> > }
>> >
>> > The current code looks like:
>> >
>> > LayoutRect LayoutText::debugRect() const
>> > {
>> > LayoutRect linesBox = linesBoundingBox();
>> > LayoutRect rect = LayoutRect(IntRect(firstRunX(), firstRunY(),
>> > linesBox.width().toInt(), linesBox.height().toInt()));
>> > ... return rect;
>> > }
>>
>> Neither of these seem ideal. Can we keep everything in LayoutUnits?
>>
>> https://codereview.chromium.org/2280153002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-09-14 17:57:34]

May I ask why we want the expected results to be all INT values?


Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Wed, Sep 14, 2016 at 1:44 PM, Luna Lu <lunalu@google.com> wrote:

> Now that I change it to
>     LayoutRect LayoutText::debugRect() const
>     {
>         LayoutRect linesBox = linesBoundingBox();
>         LayoutRect rect = LayoutRect(enclosingIntRect(
> LayoutRect(LayoutUnit(firstRunX()), LayoutUnit(firstRunY()),
> LayoutUnit(linesBox.width()), LayoutUnit(linesBox.height()))));
>         ...
>         return rect;
>     }
>
> The test results match the expectation now. But I am not so convinced this
> is the right fix. Is there anything cleaner I can do?
>
> Thanks
>
>
> Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751
>
> On Wed, Sep 14, 2016 at 12:35 PM, <skobes@chromium.org> wrote:
>
>> On 2016/09/14 14:50:04, chromium-reviews wrote:
>> > The problem comes from LayoutText.cpp. The original code looks like:
>> > LayoutRect LayoutText::debugRect() const
>> > {
>> > IntRect linesBox = enclosingIntRect(linesBoundingBox());
>> > LayoutRect rect = LayoutRect(IntRect(firstRunX(), firstRunY(),
>> > linesBox.width(), linesBox.height()));
>> > ... return rect;
>> > }
>> >
>> > The current code looks like:
>> >
>> > LayoutRect LayoutText::debugRect() const
>> > {
>> > LayoutRect linesBox = linesBoundingBox();
>> > LayoutRect rect = LayoutRect(IntRect(firstRunX(), firstRunY(),
>> > linesBox.width().toInt(), linesBox.height().toInt()));
>> > ... return rect;
>> > }
>>
>> Neither of these seem ideal. Can we keep everything in LayoutUnits?
>>
>> https://codereview.chromium.org/2280153002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[skobes, 2016-09-14 18:01:51]

On 2016/09/14 17:57:34, chromium-reviews wrote:
> May I ask why we want the expected results to be all INT values?

I don't know that we do, unless the fractional bits are indeterminate for some
reason.

How big of a rebaseline does it cause if we don't convert to int?

[blink-reviews, 2016-09-14 18:25:54]

Either .61 or .64
example:
         LayoutText {#text} at (0,0) size 271x19
         LayoutText {#text} at (0,0) size 270.39x19




Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Wed, Sep 14, 2016 at 2:01 PM, <skobes@chromium.org> wrote:

> On 2016/09/14 17:57:34, chromium-reviews wrote:
> > May I ask why we want the expected results to be all INT values?
>
> I don't know that we do, unless the fractional bits are indeterminate for
> some
> reason.
>
> How big of a rebaseline does it cause if we don't convert to int?
>
> https://codereview.chromium.org/2280153002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-09-14 18:25:54]

Either .61 or .64
example:
         LayoutText {#text} at (0,0) size 271x19
         LayoutText {#text} at (0,0) size 270.39x19




Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Wed, Sep 14, 2016 at 2:01 PM, <skobes@chromium.org> wrote:

> On 2016/09/14 17:57:34, chromium-reviews wrote:
> > May I ask why we want the expected results to be all INT values?
>
> I don't know that we do, unless the fractional bits are indeterminate for
> some
> reason.
>
> How big of a rebaseline does it cause if we don't convert to int?
>
> https://codereview.chromium.org/2280153002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[skobes, 2016-09-14 18:35:13]

On 2016/09/14 18:25:54, chromium-reviews wrote:
> Either .61 or .64
> example:
>          LayoutText {#text} at (0,0) size 271x19
>          LayoutText {#text} at (0,0) size 270.39x19

I meant, how many tests are affected?  If it's only a few, and if the output is
consistent, then I'd be in favor of updating the expectations.  But if it's a
large number, or if the output is not stable, then we should match the existing
behavior (and perhaps file a bug to follow up on the issue later).

[chromium-reviews, 2016-09-14 18:44:58]

12 + 2 tests.

Regressions: Unexpected text-only failures (12)
  fast/text/ellipsis-ltr-text-in-ltr-flow-underline-composition.html [
Failure ]
  fast/text/ellipsis-ltr-text-in-ltr-flow-underline.html [ Failure ]
  fast/text/ellipsis-ltr-text-in-ltr-flow.html [ Failure ]
  fast/text/ellipsis-ltr-text-in-rtl-flow-underline-composition.html [
Failure ]
  fast/text/ellipsis-ltr-text-in-rtl-flow-underline.html [ Failure ]
  fast/text/ellipsis-ltr-text-in-rtl-flow.html [ Failure ]
  fast/text/ellipsis-mixed-text-in-ltr-flow-underline.html [ Failure ]
  fast/text/ellipsis-mixed-text-in-rtl-flow-underline.html [ Failure ]
  fast/text/ellipsis-rtl-text-in-ltr-flow-underline.html [ Failure ]
  fast/text/ellipsis-rtl-text-in-ltr-flow.html [ Failure ]
  fast/text/ellipsis-rtl-text-in-rtl-flow-underline.html [ Failure ]
  fast/text/ellipsis-rtl-text-in-rtl-flow.html [ Failure ]

Regressions: Unexpected image and text failures (2)
  fast/text/ellipsis-rtl-text-in-ltr-flow-underline-composition.html [
Failure ]
  fast/text/ellipsis-rtl-text-in-rtl-flow-underline-composition.html [
Failure ]


Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Wed, Sep 14, 2016 at 2:35 PM, <skobes@chromium.org> wrote:

> On 2016/09/14 18:25:54, chromium-reviews wrote:
> > Either .61 or .64
> > example:
> > LayoutText {#text} at (0,0) size 271x19
> > LayoutText {#text} at (0,0) size 270.39x19
>
> I meant, how many tests are affected? If it's only a few, and if the
> output is
> consistent, then I'd be in favor of updating the expectations. But if it's
> a
> large number, or if the output is not stable, then we should match the
> existing
> behavior (and perhaps file a bug to follow up on the issue later).
>
> https://codereview.chromium.org/2280153002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blink-reviews, 2016-09-14 18:45:11]

12 + 2 tests.

Regressions: Unexpected text-only failures (12)
  fast/text/ellipsis-ltr-text-in-ltr-flow-underline-composition.html [
Failure ]
  fast/text/ellipsis-ltr-text-in-ltr-flow-underline.html [ Failure ]
  fast/text/ellipsis-ltr-text-in-ltr-flow.html [ Failure ]
  fast/text/ellipsis-ltr-text-in-rtl-flow-underline-composition.html [
Failure ]
  fast/text/ellipsis-ltr-text-in-rtl-flow-underline.html [ Failure ]
  fast/text/ellipsis-ltr-text-in-rtl-flow.html [ Failure ]
  fast/text/ellipsis-mixed-text-in-ltr-flow-underline.html [ Failure ]
  fast/text/ellipsis-mixed-text-in-rtl-flow-underline.html [ Failure ]
  fast/text/ellipsis-rtl-text-in-ltr-flow-underline.html [ Failure ]
  fast/text/ellipsis-rtl-text-in-ltr-flow.html [ Failure ]
  fast/text/ellipsis-rtl-text-in-rtl-flow-underline.html [ Failure ]
  fast/text/ellipsis-rtl-text-in-rtl-flow.html [ Failure ]

Regressions: Unexpected image and text failures (2)
  fast/text/ellipsis-rtl-text-in-ltr-flow-underline-composition.html [
Failure ]
  fast/text/ellipsis-rtl-text-in-rtl-flow-underline-composition.html [
Failure ]


Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Wed, Sep 14, 2016 at 2:35 PM, <skobes@chromium.org> wrote:

> On 2016/09/14 18:25:54, chromium-reviews wrote:
> > Either .61 or .64
> > example:
> > LayoutText {#text} at (0,0) size 271x19
> > LayoutText {#text} at (0,0) size 270.39x19
>
> I meant, how many tests are affected? If it's only a few, and if the
> output is
> consistent, then I'd be in favor of updating the expectations. But if it's
> a
> large number, or if the output is not stable, then we should match the
> existing
> behavior (and perhaps file a bug to follow up on the issue later).
>
> https://codereview.chromium.org/2280153002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[skobes, 2016-09-14 18:57:10]

Let's update the expectations.

(The image diffs will be unrelated.)

On 2016/09/14 18:45:11, blink-reviews wrote:
> 12 + 2 tests.
> 
> Regressions: Unexpected text-only failures (12)
>   fast/text/ellipsis-ltr-text-in-ltr-flow-underline-composition.html [
> Failure ]
>   fast/text/ellipsis-ltr-text-in-ltr-flow-underline.html [ Failure ]
>   fast/text/ellipsis-ltr-text-in-ltr-flow.html [ Failure ]
>   fast/text/ellipsis-ltr-text-in-rtl-flow-underline-composition.html [
> Failure ]
>   fast/text/ellipsis-ltr-text-in-rtl-flow-underline.html [ Failure ]
>   fast/text/ellipsis-ltr-text-in-rtl-flow.html [ Failure ]
>   fast/text/ellipsis-mixed-text-in-ltr-flow-underline.html [ Failure ]
>   fast/text/ellipsis-mixed-text-in-rtl-flow-underline.html [ Failure ]
>   fast/text/ellipsis-rtl-text-in-ltr-flow-underline.html [ Failure ]
>   fast/text/ellipsis-rtl-text-in-ltr-flow.html [ Failure ]
>   fast/text/ellipsis-rtl-text-in-rtl-flow-underline.html [ Failure ]
>   fast/text/ellipsis-rtl-text-in-rtl-flow.html [ Failure ]
> 
> Regressions: Unexpected image and text failures (2)
>   fast/text/ellipsis-rtl-text-in-ltr-flow-underline-composition.html [
> Failure ]
>   fast/text/ellipsis-rtl-text-in-rtl-flow-underline-composition.html [
> Failure ]
> 
> 
> Luna Lu | Software Engineer | mailto:lunalu@google.com | +1 519 513 5751
> 
> On Wed, Sep 14, 2016 at 2:35 PM, <mailto:skobes@chromium.org> wrote:
> 
> > On 2016/09/14 18:25:54, chromium-reviews wrote:
> > > Either .61 or .64
> > > example:
> > > LayoutText {#text} at (0,0) size 271x19
> > > LayoutText {#text} at (0,0) size 270.39x19
> >
> > I meant, how many tests are affected? If it's only a few, and if the
> > output is
> > consistent, then I'd be in favor of updating the expectations. But if it's
> > a
> > large number, or if the output is not stable, then we should match the
> > existing
> > behavior (and perhaps file a bug to follow up on the issue later).
> >
> > https://codereview.chromium.org/2280153002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Blink
> Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

[lunalu1, 2016-09-14 21:17:12]

Patchset #8 (id:300001) has been deleted

[lunalu1, 2016-09-14 21:18:19]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 21:19:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[blink-reviews, 2016-09-14 21:45:50]

Hi,

I am thinking about splitting this into 2 CL's. For the current one, I only
refactor (i.e., I keep the very original implementation). I will address
other FIXME's/TODO's/implementation change in the next CL.

If you think that splitting this CL is a good idea, could you please take
another look at it once I revert all the changes?

Thanks


Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Wed, Sep 14, 2016 at 2:57 PM, <skobes@chromium.org> wrote:

> Let's update the expectations.
>
> (The image diffs will be unrelated.)
>
> On 2016/09/14 18:45:11, blink-reviews wrote:
> > 12 + 2 tests.
> >
> > Regressions: Unexpected text-only failures (12)
> > fast/text/ellipsis-ltr-text-in-ltr-flow-underline-composition.html [
> > Failure ]
> > fast/text/ellipsis-ltr-text-in-ltr-flow-underline.html [ Failure ]
> > fast/text/ellipsis-ltr-text-in-ltr-flow.html [ Failure ]
> > fast/text/ellipsis-ltr-text-in-rtl-flow-underline-composition.html [
> > Failure ]
> > fast/text/ellipsis-ltr-text-in-rtl-flow-underline.html [ Failure ]
> > fast/text/ellipsis-ltr-text-in-rtl-flow.html [ Failure ]
> > fast/text/ellipsis-mixed-text-in-ltr-flow-underline.html [ Failure ]
> > fast/text/ellipsis-mixed-text-in-rtl-flow-underline.html [ Failure ]
> > fast/text/ellipsis-rtl-text-in-ltr-flow-underline.html [ Failure ]
> > fast/text/ellipsis-rtl-text-in-ltr-flow.html [ Failure ]
> > fast/text/ellipsis-rtl-text-in-rtl-flow-underline.html [ Failure ]
> > fast/text/ellipsis-rtl-text-in-rtl-flow.html [ Failure ]
> >
> > Regressions: Unexpected image and text failures (2)
> > fast/text/ellipsis-rtl-text-in-ltr-flow-underline-composition.html [
> > Failure ]
> > fast/text/ellipsis-rtl-text-in-rtl-flow-underline-composition.html [
> > Failure ]
> >
> >
> > Luna Lu | Software Engineer | mailto:lunalu@google.com | +1 519 513 5751
> >
> > On Wed, Sep 14, 2016 at 2:35 PM, <mailto:skobes@chromium.org> wrote:
> >
> > > On 2016/09/14 18:25:54, chromium-reviews wrote:
> > > > Either .61 or .64
> > > > example:
> > > > LayoutText {#text} at (0,0) size 271x19
> > > > LayoutText {#text} at (0,0) size 270.39x19
> > >
> > > I meant, how many tests are affected? If it's only a few, and if the
> > > output is
> > > consistent, then I'd be in favor of updating the expectations. But if
> it's
> > > a
> > > large number, or if the output is not stable, then we should match the
> > > existing
> > > behavior (and perhaps file a bug to follow up on the issue later).
> > >
> > > https://codereview.chromium.org/2280153002/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> "Blink
> > Reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:blink-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2280153002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-09-14 21:45:51]

Hi,

I am thinking about splitting this into 2 CL's. For the current one, I only
refactor (i.e., I keep the very original implementation). I will address
other FIXME's/TODO's/implementation change in the next CL.

If you think that splitting this CL is a good idea, could you please take
another look at it once I revert all the changes?

Thanks


Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Wed, Sep 14, 2016 at 2:57 PM, <skobes@chromium.org> wrote:

> Let's update the expectations.
>
> (The image diffs will be unrelated.)
>
> On 2016/09/14 18:45:11, blink-reviews wrote:
> > 12 + 2 tests.
> >
> > Regressions: Unexpected text-only failures (12)
> > fast/text/ellipsis-ltr-text-in-ltr-flow-underline-composition.html [
> > Failure ]
> > fast/text/ellipsis-ltr-text-in-ltr-flow-underline.html [ Failure ]
> > fast/text/ellipsis-ltr-text-in-ltr-flow.html [ Failure ]
> > fast/text/ellipsis-ltr-text-in-rtl-flow-underline-composition.html [
> > Failure ]
> > fast/text/ellipsis-ltr-text-in-rtl-flow-underline.html [ Failure ]
> > fast/text/ellipsis-ltr-text-in-rtl-flow.html [ Failure ]
> > fast/text/ellipsis-mixed-text-in-ltr-flow-underline.html [ Failure ]
> > fast/text/ellipsis-mixed-text-in-rtl-flow-underline.html [ Failure ]
> > fast/text/ellipsis-rtl-text-in-ltr-flow-underline.html [ Failure ]
> > fast/text/ellipsis-rtl-text-in-ltr-flow.html [ Failure ]
> > fast/text/ellipsis-rtl-text-in-rtl-flow-underline.html [ Failure ]
> > fast/text/ellipsis-rtl-text-in-rtl-flow.html [ Failure ]
> >
> > Regressions: Unexpected image and text failures (2)
> > fast/text/ellipsis-rtl-text-in-ltr-flow-underline-composition.html [
> > Failure ]
> > fast/text/ellipsis-rtl-text-in-rtl-flow-underline-composition.html [
> > Failure ]
> >
> >
> > Luna Lu | Software Engineer | mailto:lunalu@google.com | +1 519 513 5751
> >
> > On Wed, Sep 14, 2016 at 2:35 PM, <mailto:skobes@chromium.org> wrote:
> >
> > > On 2016/09/14 18:25:54, chromium-reviews wrote:
> > > > Either .61 or .64
> > > > example:
> > > > LayoutText {#text} at (0,0) size 271x19
> > > > LayoutText {#text} at (0,0) size 270.39x19
> > >
> > > I meant, how many tests are affected? If it's only a few, and if the
> > > output is
> > > consistent, then I'd be in favor of updating the expectations. But if
> it's
> > > a
> > > large number, or if the output is not stable, then we should match the
> > > existing
> > > behavior (and perhaps file a bug to follow up on the issue later).
> > >
> > > https://codereview.chromium.org/2280153002/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> "Blink
> > Reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:blink-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2280153002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[skobes, 2016-09-14 21:46:47]

On 2016/09/14 21:45:51, chromium-reviews wrote:
> I am thinking about splitting this into 2 CL's. For the current one, I only
> refactor (i.e., I keep the very original implementation). I will address
> other FIXME's/TODO's/implementation change in the next CL.

Yes that's fine.

[lunalu1, 2016-09-14 22:07:55]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[lunalu1, 2016-09-14 22:08:16]

The CQ bit was unchecked by lunalu@chromium.org

[lunalu1, 2016-09-14 22:11:07]

Patchset #6 (id:260001) has been deleted

[lunalu1, 2016-09-14 22:11:15]

Patchset #5 (id:240001) has been deleted

[lunalu1, 2016-09-14 22:11:32]

Patchset #5 (id:280001) has been deleted

[lunalu1, 2016-09-14 22:13:44]

Patchset #6 (id:340001) has been deleted

[lunalu1, 2016-09-14 22:14:49]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 22:15:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-09-14 22:15:37]

The CQ bit was unchecked by lunalu@chromium.org

[lunalu1, 2016-09-14 22:22:25]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[lunalu1, 2016-09-14 22:22:31]

The CQ bit was unchecked by lunalu@chromium.org

[commit-bot: I haz the power, 2016-09-14 22:22:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-09-14 22:22:49]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 22:23:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 00:40:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 00:40:49]

Dry run: This issue passed the CQ dry run.

[lunalu1, 2016-09-15 14:30:12]

Hi skobes,

I have reverted the change to the original impl. Could you please take a final
look before I submit?

Thanks

[skobes, 2016-09-15 19:04:36]

https://codereview.chromium.org/2280153002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutText.h (right):

https://codereview.chromium.org/2280153002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutText.h:184: void
applyTextTransformFromTo(int from, int len, const ComputedStyle*);
Where did this come from?

[lunalu1, 2016-09-15 20:11:09]

Done. Thanks

https://codereview.chromium.org/2280153002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutText.h (right):

https://codereview.chromium.org/2280153002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutText.h:184: void
applyTextTransformFromTo(int from, int len, const ComputedStyle*);
On 2016/09/15 19:04:36, skobes wrote:
> Where did this come from?

Done.

[skobes, 2016-09-15 20:22:19]

lgtm

[lunalu1, 2016-09-15 20:44:22]

The CQ bit was checked by lunalu@chromium.org

[lunalu1, 2016-09-15 20:44:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjhayden@chromium.org
Link to the patchset: https://codereview.chromium.org/2280153002/#ps380001
(title: "Update")

[commit-bot: I haz the power, 2016-09-15 20:45:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 21:00:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 21:00:40]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lunalu1, 2016-09-15 22:23:33]

Patchset #7 (id:380001) has been deleted

[lunalu1, 2016-09-15 22:23:42]

The CQ bit was checked by lunalu@chromium.org

[lunalu1, 2016-09-15 22:23:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjhayden@chromium.org, skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/2280153002/#ps400001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-15 22:24:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 22:37:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 22:37:50]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lunalu1, 2016-09-15 23:47:19]

Patchset #7 (id:400001) has been deleted

[lunalu1, 2016-09-15 23:48:01]

The CQ bit was checked by lunalu@chromium.org

[lunalu1, 2016-09-15 23:48:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjhayden@chromium.org, skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/2280153002/#ps420001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-15 23:48:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 01:35:41]

Committed patchset #7 (id:420001)

[commit-bot: I haz the power, 2016-09-16 01:40:05]

Description was changed from

==========
Refactoring out the code in LayoutTreeAsText::writeLayoutObject.

BUG=481588
==========

to

==========
Refactoring out the code in LayoutTreeAsText::writeLayoutObject.

BUG=481588

Committed: https://crrev.com/ecf51ddf5158be270b3ad65d613c9498d644fc6b
Cr-Commit-Position: refs/heads/master@{#419072}
==========

[commit-bot: I haz the power, 2016-09-16 01:40:07]

Patchset 7 (id:??) landed as
https://crrev.com/ecf51ddf5158be270b3ad65d613c9498d644fc6b
Cr-Commit-Position: refs/heads/master@{#419072}