Preserve autosizing multiplier on style recalc.

Preserve autosizing multiplier on style recalc.

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

[esprehn, 2014-03-22 07:46:26]

This isn't the correct place for this, the code should be in StyleResolver.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[skobes, 2014-03-24 20:35:16]

On 2014/03/22 07:46:26, esprehn wrote:
> This isn't the correct place for this, the code should be in StyleResolver.

Done, PTAL.

This change temporarily regresses
virtual/fasttextautosizing/fast/text-autosizing/resize-window.html;
http://crrev.com/209353003 should fix it.

[esprehn, 2014-03-25 01:22:21]

On 2014/03/24 20:35:16, skobes wrote:
> On 2014/03/22 07:46:26, esprehn wrote:
> > This isn't the correct place for this, the code should be in StyleResolver.
> 
> Done, PTAL.
> 
> This change temporarily regresses
> virtual/fasttextautosizing/fast/text-autosizing/resize-window.html;
> http://crrev.com/209353003 should fix it.

This won't work for pseudo elements. You also should add some tests. I think you
want to be inside StyleAdjuster? You also want to mark the style as unique to
prevent sharing with other things that might have a different multiplier.

[skobes, 2014-03-25 01:39:01]

On 2014/03/25 01:22:21, esprehn wrote:
> On 2014/03/24 20:35:16, skobes wrote:
> > On 2014/03/22 07:46:26, esprehn wrote:
> > > This isn't the correct place for this, the code should be in
StyleResolver.
> > 
> > Done, PTAL.
> > 
> > This change temporarily regresses
> > virtual/fasttextautosizing/fast/text-autosizing/resize-window.html;
> > http://crrev.com/209353003 should fix it.
> 
> This won't work for pseudo elements.

Can you elaborate?  What would we need to do for pseudo elements?

> You also should add some tests.

I'm not sure how to test this. How do I assert that something doesn't cause a
layout?

(I verified it locally using printfs. :) )

> I think you want to be inside StyleAdjuster?

Done.

> You also want to mark the style as unique to prevent sharing with other things
that might have a different multiplier.

Done.

[esprehn, 2014-03-25 02:54:27]

lgtm

[pdr., 2014-03-25 04:06:14]

On 2014/03/25 02:54:27, esprehn wrote:
> lgtm

Thanks for the detailed review esprehn.

LGTM2

[pdr., 2014-03-25 04:06:19]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-25 04:06:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skobes@chromium.org/208393008/60001

[commit-bot: I haz the power, 2014-03-25 04:08:40]

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

[commit-bot: I haz the power, 2014-03-25 04:08:41]

Try jobs failed on following builders:
  tryserver.blink on blink_presubmit

[skobes (do not use), 2014-03-25 04:13:11]

I want to add a test for this before I submit it.


On Mon, Mar 24, 2014 at 9:08 PM, <commit-bot@chromium.org> wrote:

> Try jobs failed on following builders:
>   tryserver.blink on blink_presubmit
>
> https://codereview.chromium.org/208393008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[skobes, 2014-03-25 06:57:03]

Added a test that verifies no relayout when setting color on an autosized
element.

Pseudo elements should be fixed with the change to use StyleAdjuster.

Submitting now.

[skobes, 2014-03-25 06:57:41]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-03-25 06:57:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skobes@chromium.org/208393008/70003

[commit-bot: I haz the power, 2014-03-25 06:59:40]

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

[commit-bot: I haz the power, 2014-03-25 06:59:40]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[skobes, 2014-03-25 07:02:55]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-03-25 07:03:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skobes@chromium.org/208393008/70003

[commit-bot: I haz the power, 2014-03-25 07:08:18]

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

[commit-bot: I haz the power, 2014-03-25 07:08:18]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[pdr., 2014-03-25 07:13:10]

On 2014/03/25 07:08:18, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.blink on mac_blink_rel

CQ flakes ᕙ(⇀‸↼‶)ᕗ 

Lets try again

[pdr., 2014-03-25 07:13:18]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-25 07:13:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skobes@chromium.org/208393008/70003

[commit-bot: I haz the power, 2014-03-25 07:21:36]

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

[commit-bot: I haz the power, 2014-03-25 07:21:37]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[pdr, 2014-03-25 07:24:10]

On 2014/03/25 07:21:37, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.blink on mac_blink_rel

Oh, this looks like a real compile failure after all :(

[skobes, 2014-03-25 15:33:15]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-03-25 15:33:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skobes@chromium.org/208393008/90001

[commit-bot: I haz the power, 2014-03-25 16:16:33]

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

[commit-bot: I haz the power, 2014-03-25 16:16:34]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[pdr, 2014-03-25 16:25:30]

The CQ bit was checked by pdr@google.com

[commit-bot: I haz the power, 2014-03-25 16:25:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skobes@chromium.org/208393008/90001

[commit-bot: I haz the power, 2014-03-25 17:04:35]

Change committed as 169961

[vsevik, 2014-03-26 12:12:05]

A revert of this CL has been created in
https://codereview.chromium.org/212163006/ by vsevik@chromium.org.

The reason for reverting is: This might have caused android tests failure: 
http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg...
Speculatively reverting it..

[skobes (do not use), 2014-03-26 19:20:45]

@vsevik, a couple of questions:

(1) Your revert landed as blink revision
170050<https://src.chromium.org/viewvc/blink?revision=170050&view=revision>
but
this bot went
green<http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg%29/builds/18284>at
revision 170044.  Does that mean this change wasn't the culprit?

(2) Do you know how to run androidwebview_instrumentation_tests?  The
commands in the stdio log give me the an error installing the apk:

$ build/android/test_runner.py instrumentation --test-apk
AndroidWebViewTest --test_data
webview:android_webview/test/data/device_files --verbose
--flakiness-dashboard-server=test-results.appspot.com
...
I    1s 6126  [6126]> install
/ssd1/chrome/clank/src/out/Debug/apks/AndroidWebViewTest.apk
Exception in thread 6126:
Traceback (most recent call last):
  File "/usr/lib/python2.7/threading.py", line 551, in __bootstrap_inner
    self.run()
  File
"/ssd1/chrome/clank/src/build/android/pylib/utils/reraiser_thread.py", line
70, in run
    self._func(*self._args, **self._kwargs)
  File
"/ssd1/chrome/clank/src/build/android/pylib/base/test_dispatcher.py", line
216, in _SetUp
    runner.SetUp()
  File
"/ssd1/chrome/clank/src/build/android/pylib/instrumentation/test_runner.py",
line 144, in SetUp
    super(TestRunner, self).SetUp()
  File
"/ssd1/chrome/clank/src/build/android/pylib/base/base_test_runner.py", line
80, in SetUp
    self.InstallTestPackage()
  File
"/ssd1/chrome/clank/src/build/android/pylib/instrumentation/test_runner.py",
line 92, in InstallTestPackage
    self.test_pkg.Install(self.adb)
  File
"/ssd1/chrome/clank/src/build/android/pylib/instrumentation/test_package.py",
line 37, in Install
    adb.ManagedInstall(self.GetApkPath(),
package_name=self.GetPackageName())
  File "/ssd1/chrome/clank/src/build/android/pylib/android_commands.py",
line 492, in ManagedInstall
    raise Exception('Install failure: %s' % install_status)
Exception: Install failure: 4481 KB/s (499610 bytes in 0.108s)



On Wed, Mar 26, 2014 at 5:12 AM, <vsevik@chromium.org> wrote:

> A revert of this CL has been created in
> https://codereview.chromium.org/212163006/ by vsevik@chromium.org.
>
> The reason for reverting is: This might have caused android tests failure:
> http://build.chromium.org/p/chromium.webkit/builders/
> Android%20Tests%20%28dbg...
> Speculatively reverting it..
>
> https://codereview.chromium.org/208393008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[skobes, 2014-03-26 19:21:46]

@vsevik, a couple of questions:

(1) Your revert landed as blink revision
170050<https://src.chromium.org/viewvc/blink?revision=170050&view=revision>
but
this bot went
green<http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg%29/builds/18284>
at
revision 170044.  Does that mean this change wasn't the culprit?

(2) Do you know how to run androidwebview_instrumentation_tests?  The
commands in the stdio log give me the an error installing the apk:

$ build/android/test_runner.py instrumentation --test-apk
AndroidWebViewTest --test_data
webview:android_webview/test/data/device_files --verbose
--flakiness-dashboard-server=test-results.appspot.com
...
I    1s 6126  [6126]> install
/ssd1/chrome/clank/src/out/Debug/apks/AndroidWebViewTest.apk
Exception in thread 6126:
Traceback (most recent call last):
  File "/usr/lib/python2.7/threading.py", line 551, in __bootstrap_inner
    self.run()
  File
"/ssd1/chrome/clank/src/build/android/pylib/utils/reraiser_thread.py", line
70, in run
    self._func(*self._args, **self._kwargs)
  File
"/ssd1/chrome/clank/src/build/android/pylib/base/test_dispatcher.py", line
216, in _SetUp
    runner.SetUp()
  File
"/ssd1/chrome/clank/src/build/android/pylib/instrumentation/test_runner.py",
line 144, in SetUp
    super(TestRunner, self).SetUp()
  File
"/ssd1/chrome/clank/src/build/android/pylib/base/base_test_runner.py", line
80, in SetUp
    self.InstallTestPackage()
  File
"/ssd1/chrome/clank/src/build/android/pylib/instrumentation/test_runner.py",
line 92, in InstallTestPackage
    self.test_pkg.Install(self.adb)
  File
"/ssd1/chrome/clank/src/build/android/pylib/instrumentation/test_package.py",
line 37, in Install
    adb.ManagedInstall(self.GetApkPath(),
package_name=self.GetPackageName())
  File "/ssd1/chrome/clank/src/build/android/pylib/android_commands.py",
line 492, in ManagedInstall
    raise Exception('Install failure: %s' % install_status)
Exception: Install failure: 4481 KB/s (499610 bytes in 0.108s)


On Wed, Mar 26, 2014 at 5:12 AM, <vsevik@chromium.org> wrote:

> A revert of this CL has been created in
> https://codereview.chromium.org/212163006/ by vsevik@chromium.org.
>
> The reason for reverting is: This might have caused android tests failure:
> http://build.chromium.org/p/chromium.webkit/builders/
> Android%20Tests%20%28dbg...
> Speculatively reverting it..
>
> https://codereview.chromium.org/208393008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[skobes, 2014-03-26 23:37:11]

I reproduced the error on the bot at r169961 and after the revert in r170050. 
AwSettingsTest#testLayoutAlgorithmWithTwoViews passes in portrait mode but fails
with this error in landscape mode.  When running all the instrumentation tests
together, an earlier test sometimes causes an orientation change.  We should
probably just disable this test until it is fixed properly.

[skobes, 2014-03-27 23:12:47]

On 2014/03/26 23:37:11, skobes wrote:
> I reproduced the error on the bot at r169961 and after the revert in r170050. 
> AwSettingsTest#testLayoutAlgorithmWithTwoViews passes in portrait mode but
fails
> with this error in landscape mode.  When running all the instrumentation tests
> together, an earlier test sometimes causes an orientation change.  We should
> probably just disable this test until it is fixed properly.

Turns out the failure on Nexus 7 landscape mode is a separate issue (mnaganov is
fixing it in http://crbug.com/356960).

This change broke AwSettingsTest due to a bug in
TextAutosizer::recalculateMultipliers.  The fix for that is in
http://crrev.com/214543009, after which I'll revert http://crrev.com/212163006.

[skobes, 2014-03-27 23:14:09]

On 2014/03/27 23:12:47, skobes wrote:
> On 2014/03/26 23:37:11, skobes wrote:
> > I reproduced the error on the bot at r169961 and after the revert in
r170050. 
> > AwSettingsTest#testLayoutAlgorithmWithTwoViews passes in portrait mode but
> fails
> > with this error in landscape mode.  When running all the instrumentation
tests
> > together, an earlier test sometimes causes an orientation change.  We should
> > probably just disable this test until it is fixed properly.
> 
> Turns out the failure on Nexus 7 landscape mode is a separate issue (mnaganov
is
> fixing it in http://crbug.com/356960).
> 
> This change broke AwSettingsTest due to a bug in
> TextAutosizer::recalculateMultipliers.  The fix for that is in
> http://crrev.com/214543009, after which I'll revert
http://crrev.com/212163006.

Oops, first link in the last sentence should be http://crrev.com/215063004