New issue
Advanced search Search tips

Issue 633689 link

Starred by 2 users
Status: Fixed
Owner:
sdy@chromium.org
Closed: Aug 2016
Cc:
rsesek@chromium.org
blundell@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature

Blocked on:
issue 585564


Sign in to add a comment

Refactor TabRestoreService::Entry to eliminate copying weirdness and do less manual memory management

Project Member Reported by sdy@chromium.org, Aug 2 2016 
Until recently, copying a sessions::TabRestoreService::Tab incremented its id, causing breakage (585564, 622752). Even though that's no longer the case, Tabs still have special logic to allow for copying (PlatformSpecificTabData::Clone), and code that deals with sessions::TabRestoreService::Entry in general does a lot of manual memory management.

Making Entry, Window, and Tab non-copyable, and using |unique_ptr|s for collections of them, should simplify things and inspire fewer bugs.
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 2 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/407f858ac218dc95d41e32d8752ec938b1d90ff3

commit 407f858ac218dc95d41e32d8752ec938b1d90ff3
Author: sdy <sdy@chromium.org>
Date: Tue Aug 02 21:34:35 2016

Refactor history menu tests for clearer setup

In https://crrev.com/c2aa85e42d6d85b15e46e82f2df516e2e896bd1b, I had to
fix a couple of test bugs including a wrong hard-coded array index.

This switches initialization to a hopefully-less-error-prone form.

BUG= 633689 

Review-Url: https://codereview.chromium.org/2192253002
Cr-Commit-Position: refs/heads/master@{#409331}

[modify] https://crrev.com/407f858ac218dc95d41e32d8752ec938b1d90ff3/chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 9 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13

commit 7710d22ccb2ffb236be1bc99d91e5ed5f7483a13
Author: sdy <sdy@chromium.org>
Date: Tue Aug 09 14:04:34 2016

Make TabRestoreService::Entry noncopyable and fix up surrounding code.

Instances of TabRestoreService::Entry (Window and Tab) were managed by
hand, and had special support for copying even though they never
actually need to be copied.

This refactor starts by making Entry noncopyable and branches out.

Bigger changes:
- Entry is noncopyable. PlatformSpecificTabData::Clone is gone.
- Entry is usually wrapped in unique_ptr, including inside collections.
- [Important] ValidateEntry/ValidateTab/ValidateWindow take const
  references and return a result. They don't try to fix damaged entries.

Smaller changes:
- Entry is taken by const reference whenever possible, not const pointer
  or non-const pointer/reference.
- ifs that check if an entry is a Tab and assume it's a Window otherwise
  are now switch statements.
- Loops that iterate over collections of Entry are range-based when
  appropriate.

BUG= 633689 

Review-Url: https://codereview.chromium.org/2200993004
Cr-Commit-Position: refs/heads/master@{#410655}

[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/chrome/browser/android/recently_closed_tabs_bridge.cc
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/chrome/browser/extensions/api/sessions/sessions_api.cc
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/chrome/browser/extensions/api/sessions/sessions_api.h
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/chrome/browser/sessions/persistent_tab_restore_service_browsertest.cc
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/chrome/browser/sessions/persistent_tab_restore_service_unittest.cc
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/chrome/browser/sessions/session_restore_browsertest.cc
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/chrome/browser/sessions/tab_restore_browsertest.cc
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/chrome/browser/ui/cocoa/history_menu_bridge.mm
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/chrome/browser/ui/views/frame/global_menu_bar_x11.cc
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/chrome/browser/win/jumplist.h
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/components/sessions/BUILD.gn
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/components/sessions/content/content_live_tab.cc
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/components/sessions/content/content_platform_specific_tab_data.cc
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/components/sessions/content/content_platform_specific_tab_data.h
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/components/sessions/core/in_memory_tab_restore_service.cc
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/components/sessions/core/in_memory_tab_restore_service.h
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/components/sessions/core/persistent_tab_restore_service.cc
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/components/sessions/core/persistent_tab_restore_service.h
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/components/sessions/core/tab_restore_service.cc
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/components/sessions/core/tab_restore_service.h
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/components/sessions/core/tab_restore_service_helper.cc
[modify] https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13/components/sessions/core/tab_restore_service_helper.h
[delete] https://crrev.com/041dffbe01583295125e03439bf50791296b9387/components/sessions/core/tab_restore_service_unittest.cc

Comment 3 by sdy@chromium.org, Aug 9 2016

Status: Fixed (was: Started)
►

Sign in to add a comment