New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 705339 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
OOO until July 2018
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Bug


Participants' hotlists:
Clingon-ios


Sign in to add a comment

Refactoring Bookmarks code on iOS

Project Member Reported by ramyasharma@chromium.org, Mar 27 2017

Issue description

Status: Started (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 13 2017

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

commit 742eb0b40de63b805481c7816f4de124d46211af
Author: ramyasharma <ramyasharma@chromium.org>
Date: Tue Jun 13 07:10:57 2017

Refactoring bookmark viewcontrollers

BookmarkHomeHandsetViewController and BookmarkHomeTabletNTPController
have a lot of overlap in terms of business logic, and hence results
in maintenance overhead. This effort is to refactor the classes so that
they have a common super class with the core logic.

This is CL#1: Merge existing BookmarkHomeViewController and
BookmarkHomeHandsetViewController into one class.

See go/bookmarks-refactor for more details.

BUG= 705339 

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

[modify] https://crrev.com/742eb0b40de63b805481c7816f4de124d46211af/ios/chrome/browser/ui/bookmarks/BUILD.gn
[modify] https://crrev.com/742eb0b40de63b805481c7816f4de124d46211af/ios/chrome/browser/ui/bookmarks/bookmark_collection_cells.mm
[modify] https://crrev.com/742eb0b40de63b805481c7816f4de124d46211af/ios/chrome/browser/ui/bookmarks/bookmark_controller_factory.h
[modify] https://crrev.com/742eb0b40de63b805481c7816f4de124d46211af/ios/chrome/browser/ui/bookmarks/bookmark_controller_factory.mm
[modify] https://crrev.com/742eb0b40de63b805481c7816f4de124d46211af/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.h
[modify] https://crrev.com/742eb0b40de63b805481c7816f4de124d46211af/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.mm
[rename] https://crrev.com/742eb0b40de63b805481c7816f4de124d46211af/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller_unittest.mm
[modify] https://crrev.com/742eb0b40de63b805481c7816f4de124d46211af/ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm
[delete] https://crrev.com/0ca6ede735839db5f4391fc4f14337e6f32f2c6b/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h
[delete] https://crrev.com/0ca6ede735839db5f4391fc4f14337e6f32f2c6b/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm
[modify] https://crrev.com/742eb0b40de63b805481c7816f4de124d46211af/ios/chrome/browser/ui/bookmarks/bookmark_interaction_controller.mm

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 13 2017

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

commit 6387c0e96147f491c7417ecbad7b1f79f6e32336
Author: ramyasharma <ramyasharma@chromium.org>
Date: Tue Jun 13 08:58:24 2017

Removes unnecessary subclassing of BookmarkCollectionView.

In the current code BookmarkCollectionView is only subclassed by
BookmarkFolderCollectionView. This hierarchy of classes is redundant
in the current scenario. The inheritance was introduced in legacy code,
when there were more than one subclass.

See go/bookmarks-refactor for more details.

BUG= 705339 

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

[modify] https://crrev.com/6387c0e96147f491c7417ecbad7b1f79f6e32336/ios/chrome/browser/ui/bookmarks/BUILD.gn
[modify] https://crrev.com/6387c0e96147f491c7417ecbad7b1f79f6e32336/ios/chrome/browser/ui/bookmarks/bookmark_collection_view.h
[modify] https://crrev.com/6387c0e96147f491c7417ecbad7b1f79f6e32336/ios/chrome/browser/ui/bookmarks/bookmark_collection_view.mm
[delete] https://crrev.com/89bcae17f54dc20a06d75e6c8b409d4423fabed1/ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.h
[delete] https://crrev.com/89bcae17f54dc20a06d75e6c8b409d4423fabed1/ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm
[modify] https://crrev.com/6387c0e96147f491c7417ecbad7b1f79f6e32336/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.mm
[modify] https://crrev.com/6387c0e96147f491c7417ecbad7b1f79f6e32336/ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 10 2017

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

commit 51dec2f185939a05ef01d21eccaf578b9d2ed7ea
Author: ramyasharma <ramyasharma@chromium.org>
Date: Mon Jul 10 08:55:02 2017

Creates common super class for bookmark handset and tablet view controllers

This CL just pulls out the basic views, and models into the super class. Next
CLs will pull out delegate logic, and view layouts.

BUG= 705339 

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

[modify] https://crrev.com/51dec2f185939a05ef01d21eccaf578b9d2ed7ea/ios/chrome/browser/ui/bookmarks/BUILD.gn
[modify] https://crrev.com/51dec2f185939a05ef01d21eccaf578b9d2ed7ea/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.h
[modify] https://crrev.com/51dec2f185939a05ef01d21eccaf578b9d2ed7ea/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.mm
[modify] https://crrev.com/51dec2f185939a05ef01d21eccaf578b9d2ed7ea/ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.h
[modify] https://crrev.com/51dec2f185939a05ef01d21eccaf578b9d2ed7ea/ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm
[add] https://crrev.com/51dec2f185939a05ef01d21eccaf578b9d2ed7ea/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h
[add] https://crrev.com/51dec2f185939a05ef01d21eccaf578b9d2ed7ea/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm
[add] https://crrev.com/51dec2f185939a05ef01d21eccaf578b9d2ed7ea/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_unittest.mm

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 13 2017

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

commit 1c422ade2c5e9a095c4d331a7f6d9b577f185b3f
Author: Ramya Sharma <ramyasharma@chromium.org>
Date: Thu Jul 13 02:24:07 2017

Moves folderSelector, folderEditor and editViewController to super class

This cl moves folderSelector, folderEditor and editViewController to
a common super class from bookmark handset and table view controllers.

Bug:  705339 
Change-Id: I0ed26a3a6dac8d89ff8498f2753995e4772e4a8d
Reviewed-on: https://chromium-review.googlesource.com/566778
Commit-Queue: Ramya Sharma <ramyasharma@chromium.org>
Reviewed-by: Eric Noyau <noyau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486226}
[modify] https://crrev.com/1c422ade2c5e9a095c4d331a7f6d9b577f185b3f/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.mm
[modify] https://crrev.com/1c422ade2c5e9a095c4d331a7f6d9b577f185b3f/ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm
[modify] https://crrev.com/1c422ade2c5e9a095c4d331a7f6d9b577f185b3f/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h
[modify] https://crrev.com/1c422ade2c5e9a095c4d331a7f6d9b577f185b3f/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 14 2017

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

commit ba98d2a38dfa510d02a32c9fa60a1bfb60e6942d
Author: Ramya Sharma <ramyasharma@chromium.org>
Date: Fri Jul 14 03:29:45 2017

Creates an extension for BookmarkHomeViewController for subclassing.

This CL creates a protected extension for BookmarkHomeViewController,
so that subclasses can access the protected method and read/write
properties without exposing it on the super class API.

Bug:  705339 
Change-Id: I1f6d447855ba5fbabd1676a7bc00bec38f4a1d8b
Reviewed-on: https://chromium-review.googlesource.com/569525
Commit-Queue: Ramya Sharma <ramyasharma@chromium.org>
Reviewed-by: Eric Noyau <noyau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486662}
[modify] https://crrev.com/ba98d2a38dfa510d02a32c9fa60a1bfb60e6942d/ios/chrome/browser/ui/bookmarks/BUILD.gn
[modify] https://crrev.com/ba98d2a38dfa510d02a32c9fa60a1bfb60e6942d/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.mm
[modify] https://crrev.com/ba98d2a38dfa510d02a32c9fa60a1bfb60e6942d/ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm
[modify] https://crrev.com/ba98d2a38dfa510d02a32c9fa60a1bfb60e6942d/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h
[modify] https://crrev.com/ba98d2a38dfa510d02a32c9fa60a1bfb60e6942d/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm
[add] https://crrev.com/ba98d2a38dfa510d02a32c9fa60a1bfb60e6942d/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_protected.h
[modify] https://crrev.com/ba98d2a38dfa510d02a32c9fa60a1bfb60e6942d/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_unittest.mm

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 17 2017

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

commit 64ba6f7e6ec6948ad04d35ddd87d6b0b593bf352
Author: Ramya Sharma <ramyasharma@chromium.org>
Date: Mon Jul 17 09:38:20 2017

Moves edit functionality to super class BookmarksHomeViewController.

This CL moves editing, editIndexPaths, editingBar properties and their
callbacks to super class BookmarksHomeViewController. from handset
and tablet subclasses.

Bug:705339
Change-Id: I7f1a7c5fa29aff104fbcba0d1f2a62d5647c6be2

TEST=Bookmarks functionality

Change-Id: I7f1a7c5fa29aff104fbcba0d1f2a62d5647c6be2
Reviewed-on: https://chromium-review.googlesource.com/569506
Reviewed-by: Eric Noyau <noyau@chromium.org>
Commit-Queue: Ramya Sharma <ramyasharma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487035}
[modify] https://crrev.com/64ba6f7e6ec6948ad04d35ddd87d6b0b593bf352/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.h
[modify] https://crrev.com/64ba6f7e6ec6948ad04d35ddd87d6b0b593bf352/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.mm
[modify] https://crrev.com/64ba6f7e6ec6948ad04d35ddd87d6b0b593bf352/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller_unittest.mm
[modify] https://crrev.com/64ba6f7e6ec6948ad04d35ddd87d6b0b593bf352/ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm
[modify] https://crrev.com/64ba6f7e6ec6948ad04d35ddd87d6b0b593bf352/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h
[modify] https://crrev.com/64ba6f7e6ec6948ad04d35ddd87d6b0b593bf352/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm
[modify] https://crrev.com/64ba6f7e6ec6948ad04d35ddd87d6b0b593bf352/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_protected.h

Project Member

Comment 10 by bugdroid1@chromium.org, Jul 19 2017

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

commit 69fff5cffe928185a70e61b5c4cdeffb63a1756c
Author: Ramya Sharma <ramyasharma@chromium.org>
Date: Wed Jul 19 01:17:44 2017

Moves BookmarkCollectionViewDelegate, BookmarkModelBridgeObserver to superclass

1.Moves BookmarkCollectionViewDelegate delegate method implementations and
from handset and tablet viewcontroller to common super class. 
2. Moves BookmarkModelBridgeObserver callbacks to super class.
3. Moves some navigation callbacks to super class.
4. Cleans up the protected class extension by removing methods that
are no longer used by subclasses.

Bug:  705339 
Change-Id: I5b2e462ae973574fb069603629c212f914870386
Reviewed-on: https://chromium-review.googlesource.com/573921
Commit-Queue: Ramya Sharma <ramyasharma@chromium.org>
Reviewed-by: Eric Noyau <noyau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487701}
[modify] https://crrev.com/69fff5cffe928185a70e61b5c4cdeffb63a1756c/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.h
[modify] https://crrev.com/69fff5cffe928185a70e61b5c4cdeffb63a1756c/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.mm
[modify] https://crrev.com/69fff5cffe928185a70e61b5c4cdeffb63a1756c/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller_unittest.mm
[modify] https://crrev.com/69fff5cffe928185a70e61b5c4cdeffb63a1756c/ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm
[modify] https://crrev.com/69fff5cffe928185a70e61b5c4cdeffb63a1756c/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h
[modify] https://crrev.com/69fff5cffe928185a70e61b5c4cdeffb63a1756c/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm
[modify] https://crrev.com/69fff5cffe928185a70e61b5c4cdeffb63a1756c/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_protected.h

Project Member

Comment 11 by bugdroid1@chromium.org, Jul 21 2017

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

commit b1438b5d9295b7535d750bc532be4492ae0c2e30
Author: Ramya Sharma <ramyasharma@chromium.org>
Date: Fri Jul 21 05:23:57 2017

Code cleanup of Bookmark view controllers.

This CL:
1. Removes the concept of primary view, since there is only one single
view, the folder view that is shown as content.
2. Removes code that removes and adds primary view to the view hierarchy
on each menu item update. This is not necesary, since the primary
view does not change anymore.

Bug:  705339 
Change-Id: I8f73a26a3a315cde48f319bb3a55373dd85c4cbc
Reviewed-on: https://chromium-review.googlesource.com/575305
Commit-Queue: Ramya Sharma <ramyasharma@chromium.org>
Reviewed-by: Eric Noyau <noyau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488589}
[modify] https://crrev.com/b1438b5d9295b7535d750bc532be4492ae0c2e30/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.mm
[modify] https://crrev.com/b1438b5d9295b7535d750bc532be4492ae0c2e30/ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm
[modify] https://crrev.com/b1438b5d9295b7535d750bc532be4492ae0c2e30/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm
[modify] https://crrev.com/b1438b5d9295b7535d750bc532be4492ae0c2e30/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_protected.h

Status: Fixed (was: Started)

Sign in to add a comment