GridLayout: Guard against layout queries while adding a view.

GridLayout: Guard against layout queries while adding a view.

Adds a DCHECK to GridLayout to ensure client code doesn't ask it to
perform layout calculations while inside GridLayout::AddViewState().
GridLayout has not yet updated its internal data structures, so it can
only give invalid results at this point.

Adds a BrowserDialogTest for the edit bookmark view which would have
triggered this DCHECK without the fix in r455438.

Adds a DCHECK_DEATH test for GridLayout.

BUG=699912, 699390
Review-Url: https://codereview.chromium.org/2743743003
Cr-Commit-Position: refs/heads/master@{#456638}
Committed: https://chromium.googlesource.com/chromium/src/+/4d775162580774ccfaed86a8dcf2ac6564198d93

[tapted, 2017-03-10 05:42:32]

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

[commit-bot: I haz the power, 2017-03-10 05:42:43]

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, 2017-03-10 06:22:46]

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

[commit-bot: I haz the power, 2017-03-10 06:22:47]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-03-12 23:42:29]

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

[commit-bot: I haz the power, 2017-03-12 23:42:48]

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, 2017-03-13 01:04:30]

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

[commit-bot: I haz the power, 2017-03-13 01:04:31]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-03-13 03:25:49]

Description was changed from

==========
GridLayout: Guard against layout queries while adding a view.

BUG=699912
==========

to

==========
GridLayout: Guard against layout queries while adding a view.

Adds a DCHECK to GridLayout to ensure client code doesn't ask it to
perform layout calculations while inside GridLayout::AddViewState().
GridLayout has not yet updated its internal data structures, so it can
only give invalid results at this point.

Adds a BrowserDialogTest for the edit bookmark view which would have
triggered this DCHECK without the fix in r455438.

BUG=699912, 699390
==========

[tapted, 2017-03-13 03:25:50]

tapted@chromium.org changed reviewers:
+ pkasting@chromium.org

[tapted, 2017-03-13 03:42:02]

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

[commit-bot: I haz the power, 2017-03-13 03:42:18]

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, 2017-03-13 04:30:02]

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

[commit-bot: I haz the power, 2017-03-13 04:30:02]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-03-13 06:03:31]

Description was changed from

==========
GridLayout: Guard against layout queries while adding a view.

Adds a DCHECK to GridLayout to ensure client code doesn't ask it to
perform layout calculations while inside GridLayout::AddViewState().
GridLayout has not yet updated its internal data structures, so it can
only give invalid results at this point.

Adds a BrowserDialogTest for the edit bookmark view which would have
triggered this DCHECK without the fix in r455438.

BUG=699912, 699390
==========

to

==========
GridLayout: Guard against layout queries while adding a view.

Adds a DCHECK to GridLayout to ensure client code doesn't ask it to
perform layout calculations while inside GridLayout::AddViewState().
GridLayout has not yet updated its internal data structures, so it can
only give invalid results at this point.

Adds a BrowserDialogTest for the edit bookmark view which would have
triggered this DCHECK without the fix in r455438.

Adds a DCHECK_DEATH test for GridLayout.

BUG=699912, 699390
==========

[tapted, 2017-03-13 06:19:00]

Hi Peter, please take a look

[Peter Kasting, 2017-03-13 22:40:00]

LGTM

The only thing I don't love about this is explicitly testing for the DCHECK and
calling that "guarding against" something, since a DCHECK is compiled away in
release builds and doesn't really guard anything.  It just documents a
precondition of the code, and as such, testing for it feels almost like testing
that a function has a particular comment line in it.

It makes me wonder whether there's any other way to handle this automatically,
e.g. have GridLayout() detect this case, bail out of sizing, finish the add, and
then immediately trigger a size right afterwards.

That way callers couldn't possibly get this "wrong"; but I haven't thought
through whether this would be feasible, or if it could cause any other breakage.

[tapted, 2017-03-14 00:57:46]

On 2017/03/13 22:40:00, Peter Kasting wrote:
> LGTM
> 
> The only thing I don't love about this is explicitly testing for the DCHECK
and
> calling that "guarding against" something, since a DCHECK is compiled away in
> release builds and doesn't really guard anything.  It just documents a
> precondition of the code, and as such, testing for it feels almost like
testing
> that a function has a particular comment line in it.

Yah - layout problems will always be hard to detect automatically :/. I thought
about adding a test like "make sure the edit bookmark dialog content view is at
most XX pixels" but that will break when we want to tweak dialog widths for
harmony, and be dependent on text strings, platform fonts (os versions..). So
the other part of making the DCHECK effective is just to ensure all dialogs have
"real" test coverage on bots with dchecks enabled, which is what the
BrowserDialogTest is for

> It makes me wonder whether there's any other way to handle this automatically,
> e.g. have GridLayout() detect this case, bail out of sizing, finish the add,
and
> then immediately trigger a size right afterwards.
> 
> That way callers couldn't possibly get this "wrong"; but I haven't thought
> through whether this would be feasible, or if it could cause any other
breakage.

building upon `InvalidateLayout()` might be a way to support this. (in fact
AppKit since macOS 10.11 has started making a lot of its own autolayout
"asynchronous" in the sense that [in effect] at the end of every runloop that
had a UI "transaction" a layout gets triggered only after the posted task has
completed .. although it makes diagnosing issues very hard, and it may start
biting Chrome once we can finally move past the 10.10 SDK..).

But I think we should still DCHECK in the case described regardless. The problem
with this was that it's not just GridLayout that's not ready to Layout, but
DialogClientView is also incorrectly attempting to Layout before its finished
creating itself -- and the Layout is unnecessary since it's always clobbered by
the one that Widget does during Init(), so it's always wasted work.

[tapted, 2017-03-14 00:57:56]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-03-14 00:58:13]

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, 2017-03-14 01:07:57]

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

[commit-bot: I haz the power, 2017-03-14 01:07:58]

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...)

[tapted, 2017-03-14 05:40:11]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-03-14 05:40:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2743743003/#ps60001
(title: "Rebase for BUILD.gn conflict")

[commit-bot: I haz the power, 2017-03-14 05:40:28]

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, 2017-03-14 06:41:38]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489470011336890,
"parent_rev": "d1b118ac1850cac95fa820722e227d7f7063cfdc", "commit_rev":
"4d775162580774ccfaed86a8dcf2ac6564198d93"}

[commit-bot: I haz the power, 2017-03-14 06:42:31]

Description was changed from

==========
GridLayout: Guard against layout queries while adding a view.

Adds a DCHECK to GridLayout to ensure client code doesn't ask it to
perform layout calculations while inside GridLayout::AddViewState().
GridLayout has not yet updated its internal data structures, so it can
only give invalid results at this point.

Adds a BrowserDialogTest for the edit bookmark view which would have
triggered this DCHECK without the fix in r455438.

Adds a DCHECK_DEATH test for GridLayout.

BUG=699912, 699390
==========

to

==========
GridLayout: Guard against layout queries while adding a view.

Adds a DCHECK to GridLayout to ensure client code doesn't ask it to
perform layout calculations while inside GridLayout::AddViewState().
GridLayout has not yet updated its internal data structures, so it can
only give invalid results at this point.

Adds a BrowserDialogTest for the edit bookmark view which would have
triggered this DCHECK without the fix in r455438.

Adds a DCHECK_DEATH test for GridLayout.

BUG=699912, 699390

Review-Url: https://codereview.chromium.org/2743743003
Cr-Commit-Position: refs/heads/master@{#456638}
Committed:
https://chromium.googlesource.com/chromium/src/+/4d775162580774ccfaed86a8dcf2...
==========

[commit-bot: I haz the power, 2017-03-14 06:42:32]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4d775162580774ccfaed86a8dcf2...