Issue 2946813002: Bookmark disappear first when closing browser

weidongg

Description was changed from ========== Bookmark disappear first when closing browser The CL prevent the ...

3 years, 6 months ago (2017-06-19 22:54:54 UTC) #1

weidongg

weidongg@chromium.org changed reviewers: + sky@chromium.org

3 years, 6 months ago (2017-06-19 22:55:40 UTC) #2

sky

https://codereview.chromium.org/2946813002/diff/1/chrome/browser/ui/views/frame/browser_view.cc File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2946813002/diff/1/chrome/browser/ui/views/frame/browser_view.cc#newcode2182 chrome/browser/ui/views/frame/browser_view.cc:2182: bool show_bookmark_bar = contents && I think the bug ...

3 years, 6 months ago (2017-06-20 16:35:06 UTC) #4

sky

On 2017/06/20 16:35:06, sky wrote: > https://codereview.chromium.org/2946813002/diff/1/chrome/browser/ui/views/frame/browser_view.cc > File chrome/browser/ui/views/frame/browser_view.cc (right): > > https://codereview.chromium.org/2946813002/diff/1/chrome/browser/ui/views/frame/browser_view.cc#newcode2182 > ...

3 years, 6 months ago (2017-06-20 16:35:16 UTC) #5

weidongg

In this case, I need to test whether the bookmark is detached from its parent ...

3 years, 6 months ago (2017-06-20 19:51:50 UTC) #6

weidongg

Description was changed from ========== Bookmark disappear first when closing browser The CL prevents the ...

3 years, 6 months ago (2017-06-20 21:00:07 UTC) #7

weidongg

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

3 years, 6 months ago (2017-06-20 21:01:50 UTC) #8

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-20 21:02:23 UTC) #9

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-20 22:11:20 UTC) #10

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-06-20 22:11:21 UTC) #11

sky

On Tue, Jun 20, 2017 at 12:51 PM, <weidongg@chromium.org> wrote: > In this case, I ...

3 years, 6 months ago (2017-06-21 00:22:58 UTC) #12

weidongg

On 2017/06/21 00:22:58, sky wrote: > On Tue, Jun 20, 2017 at 12:51 PM, <mailto:weidongg@chromium.org> ...

3 years, 6 months ago (2017-06-21 00:37:12 UTC) #13

sky

On 2017/06/21 00:37:12, weidongg wrote: > On 2017/06/21 00:22:58, sky wrote: > > On Tue, ...

3 years, 6 months ago (2017-06-21 14:48:01 UTC) #14

On 2017/06/21 00:37:12, weidongg wrote:
> On 2017/06/21 00:22:58, sky wrote:
> > On Tue, Jun 20, 2017 at 12:51 PM, <mailto:weidongg@chromium.org> wrote:
> > 
> > > In this case, I need to test whether the bookmark is detached from its
> > > parent at
> > > browser startup/shutdown. The parent will soon become browser view or top
> > > container after start up and the bookmark view will soon destruct after
> > > shutdown. I am not sure is there any way testing this?
> > >
> > 
> > What in particular can't you write a test for?
> > 
> > 
> > >
> > >
> > >
> > > https://codereview.chromium.org/2946813002/diff/1/chrome/
> > > browser/ui/views/frame/browser_view.cc
> > > File chrome/browser/ui/views/frame/browser_view.cc (right):
> > >
> > > https://codereview.chromium.org/2946813002/diff/1/chrome/
> > > browser/ui/views/frame/browser_view.cc#newcode2182
> > > chrome/browser/ui/views/frame/browser_view.cc:2182: bool
> > > show_bookmark_bar = contents &&
> > > On 2017/06/20 16:35:06, sky wrote:
> > > > I think the bug is here. We should show the bookmarkbar if there is no
> > > contents
> > > > (which only happens during startup/shutudown).
> > >
> > > Aha I see, so I could change it to:
> > > "bool show_bookmark_bar =
> > > browser_->SupportsWindowFeature(Browser::FEATURE_BOOKMARKBAR);"
> > >
> > > https://codereview.chromium.org/2946813002/
> > >
> > 
> > -- 
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> If I test the behavior of closing browser. The states before and after closing
> browser is
> the same. I am not sure how to test that bookmark bar view disappears not
before
> browser view.

To answer that you need to understand why this change is necessary. Why is this
change necessary? Your description says to avoid the parent being null, but that
isn't the case. Before and after this change the parent isn't null.

weidongg

Description was changed from ========== Bookmark disappear first when closing browser The CL prevents the ...

3 years, 6 months ago (2017-06-21 16:38:38 UTC) #15

weidongg

Description was changed from ========== Bookmark disappear first when closing browser The CL prevents the ...

3 years, 6 months ago (2017-06-21 16:45:09 UTC) #16

weidongg

On 2017/06/21 14:48:01, sky wrote: > On 2017/06/21 00:37:12, weidongg wrote: > > On 2017/06/21 ...

3 years, 6 months ago (2017-06-21 16:47:00 UTC) #17

On 2017/06/21 14:48:01, sky wrote:
> On 2017/06/21 00:37:12, weidongg wrote:
> > On 2017/06/21 00:22:58, sky wrote:
> > > On Tue, Jun 20, 2017 at 12:51 PM, <mailto:weidongg@chromium.org> wrote:
> > > 
> > > > In this case, I need to test whether the bookmark is detached from its
> > > > parent at
> > > > browser startup/shutdown. The parent will soon become browser view or
top
> > > > container after start up and the bookmark view will soon destruct after
> > > > shutdown. I am not sure is there any way testing this?
> > > >
> > > 
> > > What in particular can't you write a test for?
> > > 
> > > 
> > > >
> > > >
> > > >
> > > > https://codereview.chromium.org/2946813002/diff/1/chrome/
> > > > browser/ui/views/frame/browser_view.cc
> > > > File chrome/browser/ui/views/frame/browser_view.cc (right):
> > > >
> > > > https://codereview.chromium.org/2946813002/diff/1/chrome/
> > > > browser/ui/views/frame/browser_view.cc#newcode2182
> > > > chrome/browser/ui/views/frame/browser_view.cc:2182: bool
> > > > show_bookmark_bar = contents &&
> > > > On 2017/06/20 16:35:06, sky wrote:
> > > > > I think the bug is here. We should show the bookmarkbar if there is no
> > > > contents
> > > > > (which only happens during startup/shutudown).
> > > >
> > > > Aha I see, so I could change it to:
> > > > "bool show_bookmark_bar =
> > > > browser_->SupportsWindowFeature(Browser::FEATURE_BOOKMARKBAR);"
> > > >
> > > > https://codereview.chromium.org/2946813002/
> > > >
> > > 
> > > -- 
> > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> > If I test the behavior of closing browser. The states before and after
closing
> > browser is
> > the same. I am not sure how to test that bookmark bar view disappears not
> before
> > browser view.
> 
> To answer that you need to understand why this change is necessary. Why is
this
> change necessary? Your description says to avoid the parent being null, but
that
> isn't the case. Before and after this change the parent isn't null.

Updated the description.
You mentioned that the WebContents is set to null at startup and shutdown.
Before this CL, if the WebContents is null, the bookmark bar view will be
detached from its parent in MaybeShowBookmarkBar(). After this CL, the
bookmark bar view's parent remains unchanged at startup and shutdown.

weidongg

Description was changed from ========== Bookmark disappear first when closing browser The CL prevents the ...

3 years, 6 months ago (2017-06-21 16:47:24 UTC) #18

sky

On Wed, Jun 21, 2017 at 9:47 AM, <weidongg@chromium.org> wrote: > On 2017/06/21 14:48:01, sky ...

3 years, 6 months ago (2017-06-21 19:42:39 UTC) #19

On Wed, Jun 21, 2017 at 9:47 AM, <weidongg@chromium.org> wrote:

> On 2017/06/21 14:48:01, sky wrote:
> > On 2017/06/21 00:37:12, weidongg wrote:
> > > On 2017/06/21 00:22:58, sky wrote:
> > > > On Tue, Jun 20, 2017 at 12:51 PM, <mailto:weidongg@chromium.org>
> wrote:
> > > >
> > > > > In this case, I need to test whether the bookmark is detached from
> its
> > > > > parent at
> > > > > browser startup/shutdown. The parent will soon become browser view
> or
> top
> > > > > container after start up and the bookmark view will soon destruct
> after
> > > > > shutdown. I am not sure is there any way testing this?
> > > > >
> > > >
> > > > What in particular can't you write a test for?
> > > >
> > > >
> > > > >
> > > > >
> > > > >
> > > > > https://codereview.chromium.org/2946813002/diff/1/chrome/
> > > > > browser/ui/views/frame/browser_view.cc
> > > > > File chrome/browser/ui/views/frame/browser_view.cc (right):
> > > > >
> > > > > https://codereview.chromium.org/2946813002/diff/1/chrome/
> > > > > browser/ui/views/frame/browser_view.cc#newcode2182
> > > > > chrome/browser/ui/views/frame/browser_view.cc:2182: bool
> > > > > show_bookmark_bar = contents &&
> > > > > On 2017/06/20 16:35:06, sky wrote:
> > > > > > I think the bug is here. We should show the bookmarkbar if there
> is no
> > > > > contents
> > > > > > (which only happens during startup/shutudown).
> > > > >
> > > > > Aha I see, so I could change it to:
> > > > > "bool show_bookmark_bar =
> > > > > browser_->SupportsWindowFeature(Browser::FEATURE_BOOKMARKBAR);"
> > > > >
> > > > > https://codereview.chromium.org/2946813002/
> > > > >
> > > >
> > > > --
> > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > > If I test the behavior of closing browser. The states before and after
> closing
> > > browser is
> > > the same. I am not sure how to test that bookmark bar view disappears
> not
> > before
> > > browser view.
> >
> > To answer that you need to understand why this change is necessary. Why
> is
> this
> > change necessary? Your description says to avoid the parent being null,
> but
> that
> > isn't the case. Before and after this change the parent isn't null.
>
> Updated the description.
> You mentioned that the WebContents is set to null at startup and shutdown.
> Before this CL, if the WebContents is null, the bookmark bar view will be
> detached from its parent in MaybeShowBookmarkBar().
>

That's not entirely accurate. It's moved to a different parent, but the
bookmarkbarview still has a parent. So, why does that matter here? Why does
changing the parent cause the bar to be hidden? I suspect the answer is
in BrowserViewLayout::UpdateTopContainerBounds
or BrowserViewLayout::LayoutBookmarkBar.

  -Scott


> After this CL, the
> bookmark bar view's parent remains unchanged at startup and shutdown.
>
> https://codereview.chromium.org/2946813002/
>

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

weidongg

On 2017/06/21 19:42:39, sky wrote: > On Wed, Jun 21, 2017 at 9:47 AM, <mailto:weidongg@chromium.org> ...

3 years, 6 months ago (2017-06-21 22:27:51 UTC) #20

On 2017/06/21 19:42:39, sky wrote:
> On Wed, Jun 21, 2017 at 9:47 AM, <mailto:weidongg@chromium.org> wrote:
> 
> > On 2017/06/21 14:48:01, sky wrote:
> > > On 2017/06/21 00:37:12, weidongg wrote:
> > > > On 2017/06/21 00:22:58, sky wrote:
> > > > > On Tue, Jun 20, 2017 at 12:51 PM, <mailto:weidongg@chromium.org>
> > wrote:
> > > > >
> > > > > > In this case, I need to test whether the bookmark is detached from
> > its
> > > > > > parent at
> > > > > > browser startup/shutdown. The parent will soon become browser view
> > or
> > top
> > > > > > container after start up and the bookmark view will soon destruct
> > after
> > > > > > shutdown. I am not sure is there any way testing this?
> > > > > >
> > > > >
> > > > > What in particular can't you write a test for?
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > https://codereview.chromium.org/2946813002/diff/1/chrome/
> > > > > > browser/ui/views/frame/browser_view.cc
> > > > > > File chrome/browser/ui/views/frame/browser_view.cc (right):
> > > > > >
> > > > > > https://codereview.chromium.org/2946813002/diff/1/chrome/
> > > > > > browser/ui/views/frame/browser_view.cc#newcode2182
> > > > > > chrome/browser/ui/views/frame/browser_view.cc:2182: bool
> > > > > > show_bookmark_bar = contents &&
> > > > > > On 2017/06/20 16:35:06, sky wrote:
> > > > > > > I think the bug is here. We should show the bookmarkbar if there
> > is no
> > > > > > contents
> > > > > > > (which only happens during startup/shutudown).
> > > > > >
> > > > > > Aha I see, so I could change it to:
> > > > > > "bool show_bookmark_bar =
> > > > > > browser_->SupportsWindowFeature(Browser::FEATURE_BOOKMARKBAR);"
> > > > > >
> > > > > > https://codereview.chromium.org/2946813002/
> > > > > >
> > > > >
> > > > > --
> > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > > >
> > > > If I test the behavior of closing browser. The states before and after
> > closing
> > > > browser is
> > > > the same. I am not sure how to test that bookmark bar view disappears
> > not
> > > before
> > > > browser view.
> > >
> > > To answer that you need to understand why this change is necessary. Why
> > is
> > this
> > > change necessary? Your description says to avoid the parent being null,
> > but
> > that
> > > isn't the case. Before and after this change the parent isn't null.
> >
> > Updated the description.
> > You mentioned that the WebContents is set to null at startup and shutdown.
> > Before this CL, if the WebContents is null, the bookmark bar view will be
> > detached from its parent in MaybeShowBookmarkBar().
> >
> 
> That's not entirely accurate. It's moved to a different parent, but the
> bookmarkbarview still has a parent. So, why does that matter here? Why does
> changing the parent cause the bar to be hidden? I suspect the answer is
> in BrowserViewLayout::UpdateTopContainerBounds
> or BrowserViewLayout::LayoutBookmarkBar.
> 
>   -Scott
> 
> 
> > After this CL, the
> > bookmark bar view's parent remains unchanged at startup and shutdown.
> >
> > https://codereview.chromium.org/2946813002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

When close the browser, the bookmark is detached from top container and its
parent is set to nullptr [1]. So bookmark’s property visible is true, but it is
not visible (maybe) because it has no parent. But in
BrowserViewLayout::LayoutBookmarkBar, bounds is set based on the property
visible. I think this is the deep reason.
[1] https://cs.chromium.org/chromium/src/ui/views/view.cc?type=cs&l=2065

sky

I see it now. MaybeShowBookmarkBar uses new_parent = null, which unparents. ok, so then the ...

3 years, 6 months ago (2017-06-22 03:25:46 UTC) #21

I see it now. MaybeShowBookmarkBar uses new_parent = null, which unparents.
ok, so then the test should verify the bookmarkbar still has a parent after
the last webcontents is removed. Such a test should fail before your fix
and pass with your fix. Right?

  -Scott

On Wed, Jun 21, 2017 at 3:27 PM, <weidongg@chromium.org> wrote:

> On 2017/06/21 19:42:39, sky wrote:
>
> > On Wed, Jun 21, 2017 at 9:47 AM, <mailto:weidongg@chromium.org> wrote:
> >
> > > On 2017/06/21 14:48:01, sky wrote:
> > > > On 2017/06/21 00:37:12, weidongg wrote:
> > > > > On 2017/06/21 00:22:58, sky wrote:
> > > > > > On Tue, Jun 20, 2017 at 12:51 PM, <mailto:weidongg@chromium.org>
> > > wrote:
> > > > > >
> > > > > > > In this case, I need to test whether the bookmark is detached
> from
> > > its
> > > > > > > parent at
> > > > > > > browser startup/shutdown. The parent will soon become browser
> view
> > > or
> > > top
> > > > > > > container after start up and the bookmark view will soon
> destruct
> > > after
> > > > > > > shutdown. I am not sure is there any way testing this?
> > > > > > >
> > > > > >
> > > > > > What in particular can't you write a test for?
> > > > > >
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > https://codereview.chromium.org/2946813002/diff/1/chrome/
> > > > > > > browser/ui/views/frame/browser_view.cc
> > > > > > > File chrome/browser/ui/views/frame/browser_view.cc (right):
> > > > > > >
> > > > > > > https://codereview.chromium.org/2946813002/diff/1/chrome/
> > > > > > > browser/ui/views/frame/browser_view.cc#newcode2182
> > > > > > > chrome/browser/ui/views/frame/browser_view.cc:2182: bool
> > > > > > > show_bookmark_bar = contents &&
> > > > > > > On 2017/06/20 16:35:06, sky wrote:
> > > > > > > > I think the bug is here. We should show the bookmarkbar if
> there
> > > is no
> > > > > > > contents
> > > > > > > > (which only happens during startup/shutudown).
> > > > > > >
> > > > > > > Aha I see, so I could change it to:
> > > > > > > "bool show_bookmark_bar =
> > > > > > > browser_->SupportsWindowFeature(Browser:
> :FEATURE_BOOKMARKBAR);"
> > > > > > >
> > > > > > > https://codereview.chromium.org/2946813002/
> > > > > > >
> > > > > >
> > > > > > --
> > > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > >
> > > > > If I test the behavior of closing browser. The states before and
> after
> > > closing
> > > > > browser is
> > > > > the same. I am not sure how to test that bookmark bar view
> disappears
> > > not
> > > > before
> > > > > browser view.
> > > >
> > > > To answer that you need to understand why this change is necessary.
> Why
> > > is
> > > this
> > > > change necessary? Your description says to avoid the parent being
> null,
> > > but
> > > that
> > > > isn't the case. Before and after this change the parent isn't null.
> > >
> > > Updated the description.
> > > You mentioned that the WebContents is set to null at startup and
> shutdown.
> > > Before this CL, if the WebContents is null, the bookmark bar view will
> be
> > > detached from its parent in MaybeShowBookmarkBar().
> > >
> >
> > That's not entirely accurate. It's moved to a different parent, but the
> > bookmarkbarview still has a parent. So, why does that matter here? Why
> does
> > changing the parent cause the bar to be hidden? I suspect the answer is
> > in BrowserViewLayout::UpdateTopContainerBounds
> > or BrowserViewLayout::LayoutBookmarkBar.
> >
> > -Scott
> >
> >
> > > After this CL, the
> > > bookmark bar view's parent remains unchanged at startup and shutdown.
> > >
> > > https://codereview.chromium.org/2946813002/
> > >
> >
> > --
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
> When close the browser, the bookmark is detached from top container and its
> parent is set to nullptr [1]. So bookmark’s property visible is true, but
> it is
> not visible (maybe) because it has no parent. But in
> BrowserViewLayout::LayoutBookmarkBar, bounds is set based on the property
> visible. I think this is the deep reason.
> [1] https://cs.chromium.org/chromium/src/ui/views/view.cc?type=cs&l=2065
>
> https://codereview.chromium.org/2946813002/
>

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

weidongg

On 2017/06/22 03:25:46, sky wrote: > I see it now. MaybeShowBookmarkBar uses new_parent = null, ...

3 years, 6 months ago (2017-06-22 18:12:06 UTC) #22

On 2017/06/22 03:25:46, sky wrote:
> I see it now. MaybeShowBookmarkBar uses new_parent = null, which unparents.
> ok, so then the test should verify the bookmarkbar still has a parent after
> the last webcontents is removed. Such a test should fail before your fix
> and pass with your fix. Right?
> 
>   -Scott
> 
> On Wed, Jun 21, 2017 at 3:27 PM, <mailto:weidongg@chromium.org> wrote:
> 
> > On 2017/06/21 19:42:39, sky wrote:
> >
> > > On Wed, Jun 21, 2017 at 9:47 AM, <mailto:weidongg@chromium.org> wrote:
> > >
> > > > On 2017/06/21 14:48:01, sky wrote:
> > > > > On 2017/06/21 00:37:12, weidongg wrote:
> > > > > > On 2017/06/21 00:22:58, sky wrote:
> > > > > > > On Tue, Jun 20, 2017 at 12:51 PM, <mailto:weidongg@chromium.org>
> > > > wrote:
> > > > > > >
> > > > > > > > In this case, I need to test whether the bookmark is detached
> > from
> > > > its
> > > > > > > > parent at
> > > > > > > > browser startup/shutdown. The parent will soon become browser
> > view
> > > > or
> > > > top
> > > > > > > > container after start up and the bookmark view will soon
> > destruct
> > > > after
> > > > > > > > shutdown. I am not sure is there any way testing this?
> > > > > > > >
> > > > > > >
> > > > > > > What in particular can't you write a test for?
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > https://codereview.chromium.org/2946813002/diff/1/chrome/
> > > > > > > > browser/ui/views/frame/browser_view.cc
> > > > > > > > File chrome/browser/ui/views/frame/browser_view.cc (right):
> > > > > > > >
> > > > > > > > https://codereview.chromium.org/2946813002/diff/1/chrome/
> > > > > > > > browser/ui/views/frame/browser_view.cc#newcode2182
> > > > > > > > chrome/browser/ui/views/frame/browser_view.cc:2182: bool
> > > > > > > > show_bookmark_bar = contents &&
> > > > > > > > On 2017/06/20 16:35:06, sky wrote:
> > > > > > > > > I think the bug is here. We should show the bookmarkbar if
> > there
> > > > is no
> > > > > > > > contents
> > > > > > > > > (which only happens during startup/shutudown).
> > > > > > > >
> > > > > > > > Aha I see, so I could change it to:
> > > > > > > > "bool show_bookmark_bar =
> > > > > > > > browser_->SupportsWindowFeature(Browser:
> > :FEATURE_BOOKMARKBAR);"
> > > > > > > >
> > > > > > > > https://codereview.chromium.org/2946813002/
> > > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > > >
> > > > > > If I test the behavior of closing browser. The states before and
> > after
> > > > closing
> > > > > > browser is
> > > > > > the same. I am not sure how to test that bookmark bar view
> > disappears
> > > > not
> > > > > before
> > > > > > browser view.
> > > > >
> > > > > To answer that you need to understand why this change is necessary.
> > Why
> > > > is
> > > > this
> > > > > change necessary? Your description says to avoid the parent being
> > null,
> > > > but
> > > > that
> > > > > isn't the case. Before and after this change the parent isn't null.
> > > >
> > > > Updated the description.
> > > > You mentioned that the WebContents is set to null at startup and
> > shutdown.
> > > > Before this CL, if the WebContents is null, the bookmark bar view will
> > be
> > > > detached from its parent in MaybeShowBookmarkBar().
> > > >
> > >
> > > That's not entirely accurate. It's moved to a different parent, but the
> > > bookmarkbarview still has a parent. So, why does that matter here? Why
> > does
> > > changing the parent cause the bar to be hidden? I suspect the answer is
> > > in BrowserViewLayout::UpdateTopContainerBounds
> > > or BrowserViewLayout::LayoutBookmarkBar.
> > >
> > > -Scott
> > >
> > >
> > > > After this CL, the
> > > > bookmark bar view's parent remains unchanged at startup and shutdown.
> > > >
> > > > https://codereview.chromium.org/2946813002/
> > > >
> > >
> > > --
> > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> > When close the browser, the bookmark is detached from top container and its
> > parent is set to nullptr [1]. So bookmark’s property visible is true, but
> > it is
> > not visible (maybe) because it has no parent. But in
> > BrowserViewLayout::LayoutBookmarkBar, bounds is set based on the property
> > visible. I think this is the deep reason.
> > [1] https://cs.chromium.org/chromium/src/ui/views/view.cc?type=cs&l=2065
> >
> > https://codereview.chromium.org/2946813002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

IMO, my fix is not to avoid setting parent to null, but to set bookmark's height
to 0 to hide it when its parent is null. If we hide the bookmark, then contents
container will fill up the void area left by the bookmark. Thus, that void area
we see when closing the browser will not appear.

The test would be detecting the browser's size change after last webcontents is
removed.

sky

On Thu, Jun 22, 2017 at 11:12 AM, <weidongg@chromium.org> wrote: > On 2017/06/22 03:25:46, sky ...

3 years, 6 months ago (2017-06-22 20:29:39 UTC) #23

On Thu, Jun 22, 2017 at 11:12 AM, <weidongg@chromium.org> wrote:

> On 2017/06/22 03:25:46, sky wrote:
> > I see it now. MaybeShowBookmarkBar uses new_parent = null, which
> unparents.
> > ok, so then the test should verify the bookmarkbar still has a parent
> after
> > the last webcontents is removed. Such a test should fail before your fix
> > and pass with your fix. Right?
> >
> > -Scott
> >
> > On Wed, Jun 21, 2017 at 3:27 PM, <mailto:weidongg@chromium.org> wrote:
> >
> > > On 2017/06/21 19:42:39, sky wrote:
> > >
> > > > On Wed, Jun 21, 2017 at 9:47 AM, <mailto:weidongg@chromium.org>
> wrote:
> > > >
> > > > > On 2017/06/21 14:48:01, sky wrote:
> > > > > > On 2017/06/21 00:37:12, weidongg wrote:
> > > > > > > On 2017/06/21 00:22:58, sky wrote:
> > > > > > > > On Tue, Jun 20, 2017 at 12:51 PM, <mailto:
> weidongg@chromium.org>
> > > > > wrote:
> > > > > > > >
> > > > > > > > > In this case, I need to test whether the bookmark is
> detached
> > > from
> > > > > its
> > > > > > > > > parent at
> > > > > > > > > browser startup/shutdown. The parent will soon become
> browser
> > > view
> > > > > or
> > > > > top
> > > > > > > > > container after start up and the bookmark view will soon
> > > destruct
> > > > > after
> > > > > > > > > shutdown. I am not sure is there any way testing this?
> > > > > > > > >
> > > > > > > >
> > > > > > > > What in particular can't you write a test for?
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > https://codereview.chromium.org/2946813002/diff/1/chrome/
> > > > > > > > > browser/ui/views/frame/browser_view.cc
> > > > > > > > > File chrome/browser/ui/views/frame/browser_view.cc
> (right):
> > > > > > > > >
> > > > > > > > > https://codereview.chromium.org/2946813002/diff/1/chrome/
> > > > > > > > > browser/ui/views/frame/browser_view.cc#newcode2182
> > > > > > > > > chrome/browser/ui/views/frame/browser_view.cc:2182: bool
> > > > > > > > > show_bookmark_bar = contents &&
> > > > > > > > > On 2017/06/20 16:35:06, sky wrote:
> > > > > > > > > > I think the bug is here. We should show the bookmarkbar
> if
> > > there
> > > > > is no
> > > > > > > > > contents
> > > > > > > > > > (which only happens during startup/shutudown).
> > > > > > > > >
> > > > > > > > > Aha I see, so I could change it to:
> > > > > > > > > "bool show_bookmark_bar =
> > > > > > > > > browser_->SupportsWindowFeature(Browser:
> > > :FEATURE_BOOKMARKBAR);"
> > > > > > > > >
> > > > > > > > > https://codereview.chromium.org/2946813002/
> > > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > > > >
> > > > > > > If I test the behavior of closing browser. The states before
> and
> > > after
> > > > > closing
> > > > > > > browser is
> > > > > > > the same. I am not sure how to test that bookmark bar view
> > > disappears
> > > > > not
> > > > > > before
> > > > > > > browser view.
> > > > > >
> > > > > > To answer that you need to understand why this change is
> necessary.
> > > Why
> > > > > is
> > > > > this
> > > > > > change necessary? Your description says to avoid the parent being
> > > null,
> > > > > but
> > > > > that
> > > > > > isn't the case. Before and after this change the parent isn't
> null.
> > > > >
> > > > > Updated the description.
> > > > > You mentioned that the WebContents is set to null at startup and
> > > shutdown.
> > > > > Before this CL, if the WebContents is null, the bookmark bar view
> will
> > > be
> > > > > detached from its parent in MaybeShowBookmarkBar().
> > > > >
> > > >
> > > > That's not entirely accurate. It's moved to a different parent, but
> the
> > > > bookmarkbarview still has a parent. So, why does that matter here?
> Why
> > > does
> > > > changing the parent cause the bar to be hidden? I suspect the answer
> is
> > > > in BrowserViewLayout::UpdateTopContainerBounds
> > > > or BrowserViewLayout::LayoutBookmarkBar.
> > > >
> > > > -Scott
> > > >
> > > >
> > > > > After this CL, the
> > > > > bookmark bar view's parent remains unchanged at startup and
> shutdown.
> > > > >
> > > > > https://codereview.chromium.org/2946813002/
> > > > >
> > > >
> > > > --
> > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > > When close the browser, the bookmark is detached from top container
> and its
> > > parent is set to nullptr [1]. So bookmark’s property visible is true,
> but
> > > it is
> > > not visible (maybe) because it has no parent. But in
> > > BrowserViewLayout::LayoutBookmarkBar, bounds is set based on the
> property
> > > visible. I think this is the deep reason.
> > > [1] https://cs.chromium.org/chromium/src/ui/views/view.cc?
> type=cs&l=2065
> > >
> > > https://codereview.chromium.org/2946813002/
> > >
> >
> > --
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
> IMO, my fix is not to avoid setting parent to null, but to set bookmark's
> height
> to 0 to hide it when its parent is null.
>

If the parent is null the bookmark-bar is effectively hidden because it
isn't on screen.


> If we hide the bookmark, then contents
> container will fill up the void area left by the bookmark.
>

I don't understand. Can you elaborate on what you mean?

  -Scott


> Thus, that void area
> we see when closing the browser will not appear.
>
> The test would be detecting the browser's size change after last
> webcontents is
> removed.
>
>
> https://codereview.chromium.org/2946813002/
>

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

weidongg

On 2017/06/22 20:29:39, sky wrote: > On Thu, Jun 22, 2017 at 11:12 AM, <mailto:weidongg@chromium.org> ...

3 years, 6 months ago (2017-06-22 20:52:53 UTC) #24

On 2017/06/22 20:29:39, sky wrote:
> On Thu, Jun 22, 2017 at 11:12 AM, <mailto:weidongg@chromium.org> wrote:
> 
> > On 2017/06/22 03:25:46, sky wrote:
> > > I see it now. MaybeShowBookmarkBar uses new_parent = null, which
> > unparents.
> > > ok, so then the test should verify the bookmarkbar still has a parent
> > after
> > > the last webcontents is removed. Such a test should fail before your fix
> > > and pass with your fix. Right?
> > >
> > > -Scott
> > >
> > > On Wed, Jun 21, 2017 at 3:27 PM, <mailto:weidongg@chromium.org> wrote:
> > >
> > > > On 2017/06/21 19:42:39, sky wrote:
> > > >
> > > > > On Wed, Jun 21, 2017 at 9:47 AM, <mailto:weidongg@chromium.org>
> > wrote:
> > > > >
> > > > > > On 2017/06/21 14:48:01, sky wrote:
> > > > > > > On 2017/06/21 00:37:12, weidongg wrote:
> > > > > > > > On 2017/06/21 00:22:58, sky wrote:
> > > > > > > > > On Tue, Jun 20, 2017 at 12:51 PM, <mailto:
> > mailto:weidongg@chromium.org>
> > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > In this case, I need to test whether the bookmark is
> > detached
> > > > from
> > > > > > its
> > > > > > > > > > parent at
> > > > > > > > > > browser startup/shutdown. The parent will soon become
> > browser
> > > > view
> > > > > > or
> > > > > > top
> > > > > > > > > > container after start up and the bookmark view will soon
> > > > destruct
> > > > > > after
> > > > > > > > > > shutdown. I am not sure is there any way testing this?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > What in particular can't you write a test for?
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > https://codereview.chromium.org/2946813002/diff/1/chrome/
> > > > > > > > > > browser/ui/views/frame/browser_view.cc
> > > > > > > > > > File chrome/browser/ui/views/frame/browser_view.cc
> > (right):
> > > > > > > > > >
> > > > > > > > > > https://codereview.chromium.org/2946813002/diff/1/chrome/
> > > > > > > > > > browser/ui/views/frame/browser_view.cc#newcode2182
> > > > > > > > > > chrome/browser/ui/views/frame/browser_view.cc:2182: bool
> > > > > > > > > > show_bookmark_bar = contents &&
> > > > > > > > > > On 2017/06/20 16:35:06, sky wrote:
> > > > > > > > > > > I think the bug is here. We should show the bookmarkbar
> > if
> > > > there
> > > > > > is no
> > > > > > > > > > contents
> > > > > > > > > > > (which only happens during startup/shutudown).
> > > > > > > > > >
> > > > > > > > > > Aha I see, so I could change it to:
> > > > > > > > > > "bool show_bookmark_bar =
> > > > > > > > > > browser_->SupportsWindowFeature(Browser:
> > > > :FEATURE_BOOKMARKBAR);"
> > > > > > > > > >
> > > > > > > > > > https://codereview.chromium.org/2946813002/
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > > > > >
> > > > > > > > If I test the behavior of closing browser. The states before
> > and
> > > > after
> > > > > > closing
> > > > > > > > browser is
> > > > > > > > the same. I am not sure how to test that bookmark bar view
> > > > disappears
> > > > > > not
> > > > > > > before
> > > > > > > > browser view.
> > > > > > >
> > > > > > > To answer that you need to understand why this change is
> > necessary.
> > > > Why
> > > > > > is
> > > > > > this
> > > > > > > change necessary? Your description says to avoid the parent being
> > > > null,
> > > > > > but
> > > > > > that
> > > > > > > isn't the case. Before and after this change the parent isn't
> > null.
> > > > > >
> > > > > > Updated the description.
> > > > > > You mentioned that the WebContents is set to null at startup and
> > > > shutdown.
> > > > > > Before this CL, if the WebContents is null, the bookmark bar view
> > will
> > > > be
> > > > > > detached from its parent in MaybeShowBookmarkBar().
> > > > > >
> > > > >
> > > > > That's not entirely accurate. It's moved to a different parent, but
> > the
> > > > > bookmarkbarview still has a parent. So, why does that matter here?
> > Why
> > > > does
> > > > > changing the parent cause the bar to be hidden? I suspect the answer
> > is
> > > > > in BrowserViewLayout::UpdateTopContainerBounds
> > > > > or BrowserViewLayout::LayoutBookmarkBar.
> > > > >
> > > > > -Scott
> > > > >
> > > > >
> > > > > > After this CL, the
> > > > > > bookmark bar view's parent remains unchanged at startup and
> > shutdown.
> > > > > >
> > > > > > https://codereview.chromium.org/2946813002/
> > > > > >
> > > > >
> > > > > --
> > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > > >
> > > > When close the browser, the bookmark is detached from top container
> > and its
> > > > parent is set to nullptr [1]. So bookmark’s property visible is true,
> > but
> > > > it is
> > > > not visible (maybe) because it has no parent. But in
> > > > BrowserViewLayout::LayoutBookmarkBar, bounds is set based on the
> > property
> > > > visible. I think this is the deep reason.
> > > > [1] https://cs.chromium.org/chromium/src/ui/views/view.cc
> > type=cs&l=2065
> > > >
> > > > https://codereview.chromium.org/2946813002/
> > > >
> > >
> > > --
> > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> > IMO, my fix is not to avoid setting parent to null, but to set bookmark's
> > height
> > to 0 to hide it when its parent is null.
> >
> 
> If the parent is null the bookmark-bar is effectively hidden because it
> isn't on screen.
> 
> 
> > If we hide the bookmark, then contents
> > container will fill up the void area left by the bookmark.
> >
> 
> I don't understand. Can you elaborate on what you mean?
> 
>   -Scott
> 
> 
> > Thus, that void area
> > we see when closing the browser will not appear.
> >
> > The test would be detecting the browser's size change after last
> > webcontents is
> > removed.
> >
> >
> > https://codereview.chromium.org/2946813002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

Yes, sure. Before this CL, when you close the browser, the bookmark bar's parent
is set to null. Although the bookmark bar is hidden at this time, but its
property visible_ is not set to false. BrowserViewLayout::LayoutBookmarkBar [1]
only set bookmark bar's height to 0 if property visible_ is false. But the
bookmark bar's height+y is used to determine the position of the layout below
such as contents container. Thus the bookmark bar leaves a blank area when you
close the browser. (see the video in the bug thread.)

So my solution is to set bookmark bar's height to 0 when bookmark bar' parent is
null as well, so that the contents container's position can move up and fill the
blank that would have been left by the bookmark bar.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi...

sky

Wouldn't that mean with your fix we attempt to do an unnecessary layout/resize? -Scott On ...

3 years, 6 months ago (2017-06-22 21:18:05 UTC) #25

Wouldn't that mean with your fix we attempt to do an unnecessary
layout/resize?

  -Scott

On Thu, Jun 22, 2017 at 1:52 PM, <weidongg@chromium.org> wrote:

> On 2017/06/22 20:29:39, sky wrote:
>
> > On Thu, Jun 22, 2017 at 11:12 AM, <mailto:weidongg@chromium.org> wrote:
> >
> > > On 2017/06/22 03:25:46, sky wrote:
> > > > I see it now. MaybeShowBookmarkBar uses new_parent = null, which
> > > unparents.
> > > > ok, so then the test should verify the bookmarkbar still has a parent
> > > after
> > > > the last webcontents is removed. Such a test should fail before your
> fix
> > > > and pass with your fix. Right?
> > > >
> > > > -Scott
> > > >
> > > > On Wed, Jun 21, 2017 at 3:27 PM, <mailto:weidongg@chromium.org>
> wrote:
> > > >
> > > > > On 2017/06/21 19:42:39, sky wrote:
> > > > >
> > > > > > On Wed, Jun 21, 2017 at 9:47 AM, <mailto:weidongg@chromium.org>
> > > wrote:
> > > > > >
> > > > > > > On 2017/06/21 14:48:01, sky wrote:
> > > > > > > > On 2017/06/21 00:37:12, weidongg wrote:
> > > > > > > > > On 2017/06/21 00:22:58, sky wrote:
> > > > > > > > > > On Tue, Jun 20, 2017 at 12:51 PM, <mailto:
> > > mailto:weidongg@chromium.org>
> > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > In this case, I need to test whether the bookmark is
> > > detached
> > > > > from
> > > > > > > its
> > > > > > > > > > > parent at
> > > > > > > > > > > browser startup/shutdown. The parent will soon become
> > > browser
> > > > > view
> > > > > > > or
> > > > > > > top
> > > > > > > > > > > container after start up and the bookmark view will
> soon
> > > > > destruct
> > > > > > > after
> > > > > > > > > > > shutdown. I am not sure is there any way testing this?
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > What in particular can't you write a test for?
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > https://codereview.chromium.
> org/2946813002/diff/1/chrome/
> > > > > > > > > > > browser/ui/views/frame/browser_view.cc
> > > > > > > > > > > File chrome/browser/ui/views/frame/browser_view.cc
> > > (right):
> > > > > > > > > > >
> > > > > > > > > > > https://codereview.chromium.
> org/2946813002/diff/1/chrome/
> > > > > > > > > > > browser/ui/views/frame/browser_view.cc#newcode2182
> > > > > > > > > > > chrome/browser/ui/views/frame/browser_view.cc:2182:
> bool
> > > > > > > > > > > show_bookmark_bar = contents &&
> > > > > > > > > > > On 2017/06/20 16:35:06, sky wrote:
> > > > > > > > > > > > I think the bug is here. We should show the
> bookmarkbar
> > > if
> > > > > there
> > > > > > > is no
> > > > > > > > > > > contents
> > > > > > > > > > > > (which only happens during startup/shutudown).
> > > > > > > > > > >
> > > > > > > > > > > Aha I see, so I could change it to:
> > > > > > > > > > > "bool show_bookmark_bar =
> > > > > > > > > > > browser_->SupportsWindowFeature(Browser:
> > > > > :FEATURE_BOOKMARKBAR);"
> > > > > > > > > > >
> > > > > > > > > > > https://codereview.chromium.org/2946813002/
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > > > > > >
> > > > > > > > > If I test the behavior of closing browser. The states
> before
> > > and
> > > > > after
> > > > > > > closing
> > > > > > > > > browser is
> > > > > > > > > the same. I am not sure how to test that bookmark bar view
> > > > > disappears
> > > > > > > not
> > > > > > > > before
> > > > > > > > > browser view.
> > > > > > > >
> > > > > > > > To answer that you need to understand why this change is
> > > necessary.
> > > > > Why
> > > > > > > is
> > > > > > > this
> > > > > > > > change necessary? Your description says to avoid the parent
> being
> > > > > null,
> > > > > > > but
> > > > > > > that
> > > > > > > > isn't the case. Before and after this change the parent isn't
> > > null.
> > > > > > >
> > > > > > > Updated the description.
> > > > > > > You mentioned that the WebContents is set to null at startup
> and
> > > > > shutdown.
> > > > > > > Before this CL, if the WebContents is null, the bookmark bar
> view
> > > will
> > > > > be
> > > > > > > detached from its parent in MaybeShowBookmarkBar().
> > > > > > >
> > > > > >
> > > > > > That's not entirely accurate. It's moved to a different parent,
> but
> > > the
> > > > > > bookmarkbarview still has a parent. So, why does that matter
> here?
> > > Why
> > > > > does
> > > > > > changing the parent cause the bar to be hidden? I suspect the
> answer
> > > is
> > > > > > in BrowserViewLayout::UpdateTopContainerBounds
> > > > > > or BrowserViewLayout::LayoutBookmarkBar.
> > > > > >
> > > > > > -Scott
> > > > > >
> > > > > >
> > > > > > > After this CL, the
> > > > > > > bookmark bar view's parent remains unchanged at startup and
> > > shutdown.
> > > > > > >
> > > > > > > https://codereview.chromium.org/2946813002/
> > > > > > >
> > > > > >
> > > > > > --
> > > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > >
> > > > > When close the browser, the bookmark is detached from top container
> > > and its
> > > > > parent is set to nullptr [1]. So bookmark’s property visible is
> true,
> > > but
> > > > > it is
> > > > > not visible (maybe) because it has no parent. But in
> > > > > BrowserViewLayout::LayoutBookmarkBar, bounds is set based on the
> > > property
> > > > > visible. I think this is the deep reason.
> > > > > [1] https://cs.chromium.org/chromium/src/ui/views/view.cc
> > > type=cs&l=2065
> > > > >
> > > > > https://codereview.chromium.org/2946813002/
> > > > >
> > > >
> > > > --
> > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > > IMO, my fix is not to avoid setting parent to null, but to set
> bookmark's
> > > height
> > > to 0 to hide it when its parent is null.
> > >
> >
> > If the parent is null the bookmark-bar is effectively hidden because it
> > isn't on screen.
> >
> >
> > > If we hide the bookmark, then contents
> > > container will fill up the void area left by the bookmark.
> > >
> >
> > I don't understand. Can you elaborate on what you mean?
> >
> > -Scott
> >
> >
> > > Thus, that void area
> > > we see when closing the browser will not appear.
> > >
> > > The test would be detecting the browser's size change after last
> > > webcontents is
> > > removed.
> > >
> > >
> > > https://codereview.chromium.org/2946813002/
> > >
> >
> > --
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
> Yes, sure. Before this CL, when you close the browser, the bookmark bar's
> parent
> is set to null. Although the bookmark bar is hidden at this time, but its
> property visible_ is not set to false. BrowserViewLayout::LayoutBookmarkBar
> [1]
> only set bookmark bar's height to 0 if property visible_ is false. But the
> bookmark bar's height+y is used to determine the position of the layout
> below
> such as contents container. Thus the bookmark bar leaves a blank area when
> you
> close the browser. (see the video in the bug thread.)
>
> So my solution is to set bookmark bar's height to 0 when bookmark bar'
> parent is
> null as well, so that the contents container's position can move up and
> fill the
> blank that would have been left by the bookmark bar.
>
> [1]
> https://cs.chromium.org/chromium/src/chrome/browser/
> ui/views/frame/browser_view_layout.cc?q=browser_view_layout&dr=CSs&l=428
>
> https://codereview.chromium.org/2946813002/
>

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

weidongg

On 2017/06/22 21:18:05, sky wrote: > Wouldn't that mean with your fix we attempt to ...

3 years, 6 months ago (2017-06-23 03:03:24 UTC) #26

On 2017/06/22 21:18:05, sky wrote:
> Wouldn't that mean with your fix we attempt to do an unnecessary
> layout/resize?
> 
>   -Scott
> 
> On Thu, Jun 22, 2017 at 1:52 PM, <mailto:weidongg@chromium.org> wrote:
> 
> > On 2017/06/22 20:29:39, sky wrote:
> >
> > > On Thu, Jun 22, 2017 at 11:12 AM, <mailto:weidongg@chromium.org> wrote:
> > >
> > > > On 2017/06/22 03:25:46, sky wrote:
> > > > > I see it now. MaybeShowBookmarkBar uses new_parent = null, which
> > > > unparents.
> > > > > ok, so then the test should verify the bookmarkbar still has a parent
> > > > after
> > > > > the last webcontents is removed. Such a test should fail before your
> > fix
> > > > > and pass with your fix. Right?
> > > > >
> > > > > -Scott
> > > > >
> > > > > On Wed, Jun 21, 2017 at 3:27 PM, <mailto:weidongg@chromium.org>
> > wrote:
> > > > >
> > > > > > On 2017/06/21 19:42:39, sky wrote:
> > > > > >
> > > > > > > On Wed, Jun 21, 2017 at 9:47 AM, <mailto:weidongg@chromium.org>
> > > > wrote:
> > > > > > >
> > > > > > > > On 2017/06/21 14:48:01, sky wrote:
> > > > > > > > > On 2017/06/21 00:37:12, weidongg wrote:
> > > > > > > > > > On 2017/06/21 00:22:58, sky wrote:
> > > > > > > > > > > On Tue, Jun 20, 2017 at 12:51 PM, <mailto:
> > > > mailto:weidongg@chromium.org>
> > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > In this case, I need to test whether the bookmark is
> > > > detached
> > > > > > from
> > > > > > > > its
> > > > > > > > > > > > parent at
> > > > > > > > > > > > browser startup/shutdown. The parent will soon become
> > > > browser
> > > > > > view
> > > > > > > > or
> > > > > > > > top
> > > > > > > > > > > > container after start up and the bookmark view will
> > soon
> > > > > > destruct
> > > > > > > > after
> > > > > > > > > > > > shutdown. I am not sure is there any way testing this?
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > What in particular can't you write a test for?
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > https://codereview.chromium.
> > org/2946813002/diff/1/chrome/
> > > > > > > > > > > > browser/ui/views/frame/browser_view.cc
> > > > > > > > > > > > File chrome/browser/ui/views/frame/browser_view.cc
> > > > (right):
> > > > > > > > > > > >
> > > > > > > > > > > > https://codereview.chromium.
> > org/2946813002/diff/1/chrome/
> > > > > > > > > > > > browser/ui/views/frame/browser_view.cc#newcode2182
> > > > > > > > > > > > chrome/browser/ui/views/frame/browser_view.cc:2182:
> > bool
> > > > > > > > > > > > show_bookmark_bar = contents &&
> > > > > > > > > > > > On 2017/06/20 16:35:06, sky wrote:
> > > > > > > > > > > > > I think the bug is here. We should show the
> > bookmarkbar
> > > > if
> > > > > > there
> > > > > > > > is no
> > > > > > > > > > > > contents
> > > > > > > > > > > > > (which only happens during startup/shutudown).
> > > > > > > > > > > >
> > > > > > > > > > > > Aha I see, so I could change it to:
> > > > > > > > > > > > "bool show_bookmark_bar =
> > > > > > > > > > > > browser_->SupportsWindowFeature(Browser:
> > > > > > :FEATURE_BOOKMARKBAR);"
> > > > > > > > > > > >
> > > > > > > > > > > > https://codereview.chromium.org/2946813002/
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > --
> > > > > > > > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > > > > > > >
> > > > > > > > > > If I test the behavior of closing browser. The states
> > before
> > > > and
> > > > > > after
> > > > > > > > closing
> > > > > > > > > > browser is
> > > > > > > > > > the same. I am not sure how to test that bookmark bar view
> > > > > > disappears
> > > > > > > > not
> > > > > > > > > before
> > > > > > > > > > browser view.
> > > > > > > > >
> > > > > > > > > To answer that you need to understand why this change is
> > > > necessary.
> > > > > > Why
> > > > > > > > is
> > > > > > > > this
> > > > > > > > > change necessary? Your description says to avoid the parent
> > being
> > > > > > null,
> > > > > > > > but
> > > > > > > > that
> > > > > > > > > isn't the case. Before and after this change the parent isn't
> > > > null.
> > > > > > > >
> > > > > > > > Updated the description.
> > > > > > > > You mentioned that the WebContents is set to null at startup
> > and
> > > > > > shutdown.
> > > > > > > > Before this CL, if the WebContents is null, the bookmark bar
> > view
> > > > will
> > > > > > be
> > > > > > > > detached from its parent in MaybeShowBookmarkBar().
> > > > > > > >
> > > > > > >
> > > > > > > That's not entirely accurate. It's moved to a different parent,
> > but
> > > > the
> > > > > > > bookmarkbarview still has a parent. So, why does that matter
> > here?
> > > > Why
> > > > > > does
> > > > > > > changing the parent cause the bar to be hidden? I suspect the
> > answer
> > > > is
> > > > > > > in BrowserViewLayout::UpdateTopContainerBounds
> > > > > > > or BrowserViewLayout::LayoutBookmarkBar.
> > > > > > >
> > > > > > > -Scott
> > > > > > >
> > > > > > >
> > > > > > > > After this CL, the
> > > > > > > > bookmark bar view's parent remains unchanged at startup and
> > > > shutdown.
> > > > > > > >
> > > > > > > > https://codereview.chromium.org/2946813002/
> > > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > > >
> > > > > > When close the browser, the bookmark is detached from top container
> > > > and its
> > > > > > parent is set to nullptr [1]. So bookmark’s property visible is
> > true,
> > > > but
> > > > > > it is
> > > > > > not visible (maybe) because it has no parent. But in
> > > > > > BrowserViewLayout::LayoutBookmarkBar, bounds is set based on the
> > > > property
> > > > > > visible. I think this is the deep reason.
> > > > > > [1] https://cs.chromium.org/chromium/src/ui/views/view.cc
> > > > type=cs&l=2065
> > > > > >
> > > > > > https://codereview.chromium.org/2946813002/
> > > > > >
> > > > >
> > > > > --
> > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > > >
> > > > IMO, my fix is not to avoid setting parent to null, but to set
> > bookmark's
> > > > height
> > > > to 0 to hide it when its parent is null.
> > > >
> > >
> > > If the parent is null the bookmark-bar is effectively hidden because it
> > > isn't on screen.
> > >
> > >
> > > > If we hide the bookmark, then contents
> > > > container will fill up the void area left by the bookmark.
> > > >
> > >
> > > I don't understand. Can you elaborate on what you mean?
> > >
> > > -Scott
> > >
> > >
> > > > Thus, that void area
> > > > we see when closing the browser will not appear.
> > > >
> > > > The test would be detecting the browser's size change after last
> > > > webcontents is
> > > > removed.
> > > >
> > > >
> > > > https://codereview.chromium.org/2946813002/
> > > >
> > >
> > > --
> > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> > Yes, sure. Before this CL, when you close the browser, the bookmark bar's
> > parent
> > is set to null. Although the bookmark bar is hidden at this time, but its
> > property visible_ is not set to false. BrowserViewLayout::LayoutBookmarkBar
> > [1]
> > only set bookmark bar's height to 0 if property visible_ is false. But the
> > bookmark bar's height+y is used to determine the position of the layout
> > below
> > such as contents container. Thus the bookmark bar leaves a blank area when
> > you
> > close the browser. (see the video in the bug thread.)
> >
> > So my solution is to set bookmark bar's height to 0 when bookmark bar'
> > parent is
> > null as well, so that the contents container's position can move up and
> > fill the
> > blank that would have been left by the bookmark bar.
> >
> > [1]
> > https://cs.chromium.org/chromium/src/chrome/browser/
> > ui/views/frame/browser_view_layout.cc?q=browser_view_layout&dr=CSs&l=428
> >
> > https://codereview.chromium.org/2946813002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

Yes, you are right. But this can avoid the blank area. The other solution is to
avoid setting parent to be null when web contents is null. But I am not sure
whether the change would affect other behavior. Could you give some suggestions?
other behavior.

sky

I like avoiding setting the parent to null. As I mentioned early that seems like ...

3 years, 6 months ago (2017-06-23 03:13:36 UTC) #27

I like avoiding setting the parent to null. As I mentioned early that seems
like the right fix to me. And you should easily be able to write a test for
it.

  -Scott

On Thu, Jun 22, 2017 at 8:03 PM, <weidongg@chromium.org> wrote:

> On 2017/06/22 21:18:05, sky wrote:
> > Wouldn't that mean with your fix we attempt to do an unnecessary
> > layout/resize?
> >
> > -Scott
> >
> > On Thu, Jun 22, 2017 at 1:52 PM, <mailto:weidongg@chromium.org> wrote:
> >
> > > On 2017/06/22 20:29:39, sky wrote:
> > >
> > > > On Thu, Jun 22, 2017 at 11:12 AM, <mailto:weidongg@chromium.org>
> wrote:
> > > >
> > > > > On 2017/06/22 03:25:46, sky wrote:
> > > > > > I see it now. MaybeShowBookmarkBar uses new_parent = null, which
> > > > > unparents.
> > > > > > ok, so then the test should verify the bookmarkbar still has a
> parent
> > > > > after
> > > > > > the last webcontents is removed. Such a test should fail before
> your
> > > fix
> > > > > > and pass with your fix. Right?
> > > > > >
> > > > > > -Scott
> > > > > >
> > > > > > On Wed, Jun 21, 2017 at 3:27 PM, <mailto:weidongg@chromium.org>
> > > wrote:
> > > > > >
> > > > > > > On 2017/06/21 19:42:39, sky wrote:
> > > > > > >
> > > > > > > > On Wed, Jun 21, 2017 at 9:47 AM, <mailto:
> weidongg@chromium.org>
> > > > > wrote:
> > > > > > > >
> > > > > > > > > On 2017/06/21 14:48:01, sky wrote:
> > > > > > > > > > On 2017/06/21 00:37:12, weidongg wrote:
> > > > > > > > > > > On 2017/06/21 00:22:58, sky wrote:
> > > > > > > > > > > > On Tue, Jun 20, 2017 at 12:51 PM, <mailto:
> > > > > mailto:weidongg@chromium.org>
> > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > > In this case, I need to test whether the bookmark
> is
> > > > > detached
> > > > > > > from
> > > > > > > > > its
> > > > > > > > > > > > > parent at
> > > > > > > > > > > > > browser startup/shutdown. The parent will soon
> become
> > > > > browser
> > > > > > > view
> > > > > > > > > or
> > > > > > > > > top
> > > > > > > > > > > > > container after start up and the bookmark view will
> > > soon
> > > > > > > destruct
> > > > > > > > > after
> > > > > > > > > > > > > shutdown. I am not sure is there any way testing
> this?
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > What in particular can't you write a test for?
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > https://codereview.chromium.
> > > org/2946813002/diff/1/chrome/
> > > > > > > > > > > > > browser/ui/views/frame/browser_view.cc
> > > > > > > > > > > > > File chrome/browser/ui/views/frame/browser_view.cc
> > > > > (right):
> > > > > > > > > > > > >
> > > > > > > > > > > > > https://codereview.chromium.
> > > org/2946813002/diff/1/chrome/
> > > > > > > > > > > > > browser/ui/views/frame/browser_view.cc#newcode2182
> > > > > > > > > > > > > chrome/browser/ui/views/frame/
> browser_view.cc:2182:
> > > bool
> > > > > > > > > > > > > show_bookmark_bar = contents &&
> > > > > > > > > > > > > On 2017/06/20 16:35:06, sky wrote:
> > > > > > > > > > > > > > I think the bug is here. We should show the
> > > bookmarkbar
> > > > > if
> > > > > > > there
> > > > > > > > > is no
> > > > > > > > > > > > > contents
> > > > > > > > > > > > > > (which only happens during startup/shutudown).
> > > > > > > > > > > > >
> > > > > > > > > > > > > Aha I see, so I could change it to:
> > > > > > > > > > > > > "bool show_bookmark_bar =
> > > > > > > > > > > > > browser_->SupportsWindowFeature(Browser:
> > > > > > > :FEATURE_BOOKMARKBAR);"
> > > > > > > > > > > > >
> > > > > > > > > > > > > https://codereview.chromium.org/2946813002/
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > --
> > > > > > > > > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > > > > > > > >
> > > > > > > > > > > If I test the behavior of closing browser. The states
> > > before
> > > > > and
> > > > > > > after
> > > > > > > > > closing
> > > > > > > > > > > browser is
> > > > > > > > > > > the same. I am not sure how to test that bookmark bar
> view
> > > > > > > disappears
> > > > > > > > > not
> > > > > > > > > > before
> > > > > > > > > > > browser view.
> > > > > > > > > >
> > > > > > > > > > To answer that you need to understand why this change is
> > > > > necessary.
> > > > > > > Why
> > > > > > > > > is
> > > > > > > > > this
> > > > > > > > > > change necessary? Your description says to avoid the
> parent
> > > being
> > > > > > > null,
> > > > > > > > > but
> > > > > > > > > that
> > > > > > > > > > isn't the case. Before and after this change the parent
> isn't
> > > > > null.
> > > > > > > > >
> > > > > > > > > Updated the description.
> > > > > > > > > You mentioned that the WebContents is set to null at
> startup
> > > and
> > > > > > > shutdown.
> > > > > > > > > Before this CL, if the WebContents is null, the bookmark
> bar
> > > view
> > > > > will
> > > > > > > be
> > > > > > > > > detached from its parent in MaybeShowBookmarkBar().
> > > > > > > > >
> > > > > > > >
> > > > > > > > That's not entirely accurate. It's moved to a different
> parent,
> > > but
> > > > > the
> > > > > > > > bookmarkbarview still has a parent. So, why does that matter
> > > here?
> > > > > Why
> > > > > > > does
> > > > > > > > changing the parent cause the bar to be hidden? I suspect the
> > > answer
> > > > > is
> > > > > > > > in BrowserViewLayout::UpdateTopContainerBounds
> > > > > > > > or BrowserViewLayout::LayoutBookmarkBar.
> > > > > > > >
> > > > > > > > -Scott
> > > > > > > >
> > > > > > > >
> > > > > > > > > After this CL, the
> > > > > > > > > bookmark bar view's parent remains unchanged at startup and
> > > > > shutdown.
> > > > > > > > >
> > > > > > > > > https://codereview.chromium.org/2946813002/
> > > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > > > >
> > > > > > > When close the browser, the bookmark is detached from top
> container
> > > > > and its
> > > > > > > parent is set to nullptr [1]. So bookmark’s property visible is
> > > true,
> > > > > but
> > > > > > > it is
> > > > > > > not visible (maybe) because it has no parent. But in
> > > > > > > BrowserViewLayout::LayoutBookmarkBar, bounds is set based on
> the
> > > > > property
> > > > > > > visible. I think this is the deep reason.
> > > > > > > [1] https://cs.chromium.org/chromium/src/ui/views/view.cc
> > > > > type=cs&l=2065
> > > > > > >
> > > > > > > https://codereview.chromium.org/2946813002/
> > > > > > >
> > > > > >
> > > > > > --
> > > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > >
> > > > > IMO, my fix is not to avoid setting parent to null, but to set
> > > bookmark's
> > > > > height
> > > > > to 0 to hide it when its parent is null.
> > > > >
> > > >
> > > > If the parent is null the bookmark-bar is effectively hidden because
> it
> > > > isn't on screen.
> > > >
> > > >
> > > > > If we hide the bookmark, then contents
> > > > > container will fill up the void area left by the bookmark.
> > > > >
> > > >
> > > > I don't understand. Can you elaborate on what you mean?
> > > >
> > > > -Scott
> > > >
> > > >
> > > > > Thus, that void area
> > > > > we see when closing the browser will not appear.
> > > > >
> > > > > The test would be detecting the browser's size change after last
> > > > > webcontents is
> > > > > removed.
> > > > >
> > > > >
> > > > > https://codereview.chromium.org/2946813002/
> > > > >
> > > >
> > > > --
> > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > > Yes, sure. Before this CL, when you close the browser, the bookmark
> bar's
> > > parent
> > > is set to null. Although the bookmark bar is hidden at this time, but
> its
> > > property visible_ is not set to false. BrowserViewLayout::
> LayoutBookmarkBar
> > > [1]
> > > only set bookmark bar's height to 0 if property visible_ is false. But
> the
> > > bookmark bar's height+y is used to determine the position of the layout
> > > below
> > > such as contents container. Thus the bookmark bar leaves a blank area
> when
> > > you
> > > close the browser. (see the video in the bug thread.)
> > >
> > > So my solution is to set bookmark bar's height to 0 when bookmark bar'
> > > parent is
> > > null as well, so that the contents container's position can move up and
> > > fill the
> > > blank that would have been left by the bookmark bar.
> > >
> > > [1]
> > > https://cs.chromium.org/chromium/src/chrome/browser/
> > > ui/views/frame/browser_view_layout.cc?q=browser_view_
> layout&dr=CSs&l=428
> > >
> > > https://codereview.chromium.org/2946813002/
> > >
> >
> > --
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
> Yes, you are right. But this can avoid the blank area. The other solution
> is to
> avoid setting parent to be null when web contents is null. But I am not
> sure
> whether the change would affect other behavior. Could you give some
> suggestions?
> other behavior.
>
> https://codereview.chromium.org/2946813002/
>

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

weidongg

Description was changed from ========== Bookmark disappear first when closing browser The CL prevents the ...

3 years, 6 months ago (2017-06-23 23:04:27 UTC) #30

weidongg

Ok, I uploaded a new patch set and added unit test. Thanks.

3 years, 6 months ago (2017-06-23 23:07:55 UTC) #31

sky

https://codereview.chromium.org/2946813002/diff/80001/chrome/browser/ui/views/frame/browser_view.cc File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2946813002/diff/80001/chrome/browser/ui/views/frame/browser_view.cc#newcode2173 chrome/browser/ui/views/frame/browser_view.cc:2173: bool show_bookmark_bar = contents && I liked your earlier ...

3 years, 5 months ago (2017-06-26 15:28:34 UTC) #32

weidongg

Yes, I agree. I uploaded a new patch set to return back to the earlier ...

3 years, 5 months ago (2017-06-26 16:49:14 UTC) #33

commit-bot: I haz the power

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

3 years, 5 months ago (2017-06-26 20:51:45 UTC) #36

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498510269484450, "parent_rev": "80082bfbdb7506d138d75fc87ec40be5d246f1c6", "commit_rev": "b5cae6d3aaf71d0f6186504da2f50fef77d20982"}

3 years, 5 months ago (2017-06-26 21:51:54 UTC) #37

commit-bot: I haz the power

Description was changed from ========== Bookmark disappear first when closing browser The CL prevents the ...

3 years, 5 months ago (2017-06-26 21:52:07 UTC) #38

commit-bot: I haz the power

3 years, 5 months ago (2017-06-26 21:52:08 UTC) #39

Message was sent while issue was closed.

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/b5cae6d3aaf71d0f6186504da2f5...

Issue 2946813002: Bookmark disappear first when closing browser (Closed)

Description

Patch Set 1 #

Patch Set 2 : Change solution #

Patch Set 3 : Please ignore this patch set #

Patch Set 4 : Deep reason #

Patch Set 5 : Add unittest #

Patch Set 6 : Return back to earlier fix #

Messages