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

Issue 737110 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Task



Sign in to add a comment

Have a better atomicity of responsibilities in ContentSuggestions

Project Member Reported by gambard@chromium.org, Jun 27 2017

Issue description

Some objects in ContentSuggestions (coordinator, mediator...) are doing too many things. Some of their responsibilities should be split in multiple objects.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 29 2017

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

commit a40a5b97f923577b37a877f89c9bc1934ae11846
Author: gambard <gambard@chromium.org>
Date: Thu Jun 29 15:22:22 2017

Move the ContentSuggestions alerts to an object

The ContentSuggestionsCoordinator is doing too many things.
This CL mitigates this by moving the creations of the alerts
to another class.

BUG= 737110 

Change-Id: I08fc34829f21c5e9daa504ec80a5a4575d656828
Reviewed-on: https://chromium-review.googlesource.com/550036
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Louis Romero <lpromero@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483367}
[modify] https://crrev.com/a40a5b97f923577b37a877f89c9bc1934ae11846/ios/chrome/browser/content_suggestions/BUILD.gn
[add] https://crrev.com/a40a5b97f923577b37a877f89c9bc1934ae11846/ios/chrome/browser/content_suggestions/content_suggestions_alert_commands.h
[add] https://crrev.com/a40a5b97f923577b37a877f89c9bc1934ae11846/ios/chrome/browser/content_suggestions/content_suggestions_alert_egtest.mm
[add] https://crrev.com/a40a5b97f923577b37a877f89c9bc1934ae11846/ios/chrome/browser/content_suggestions/content_suggestions_alert_factory.h
[add] https://crrev.com/a40a5b97f923577b37a877f89c9bc1934ae11846/ios/chrome/browser/content_suggestions/content_suggestions_alert_factory.mm
[modify] https://crrev.com/a40a5b97f923577b37a877f89c9bc1934ae11846/ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm
[modify] https://crrev.com/a40a5b97f923577b37a877f89c9bc1934ae11846/ios/chrome/test/earl_grey/BUILD.gn

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 29 2017

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

commit b4519a14f8ecb2b6a0ca5131203741e38ec10796
Author: Rohit Rao <rohitrao@chromium.org>
Date: Thu Jun 29 16:48:18 2017

Revert "Move the ContentSuggestions alerts to an object"

This reverts commit a40a5b97f923577b37a877f89c9bc1934ae11846.

Reason for revert: Breaks egtests on ipad air 2 ios 10.0.

Original change's description:
> Move the ContentSuggestions alerts to an object
> 
> The ContentSuggestionsCoordinator is doing too many things.
> This CL mitigates this by moving the creations of the alerts
> to another class.
> 
> BUG= 737110 
> 
> Change-Id: I08fc34829f21c5e9daa504ec80a5a4575d656828
> Reviewed-on: https://chromium-review.googlesource.com/550036
> Commit-Queue: Gauthier Ambard <gambard@chromium.org>
> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
> Reviewed-by: Louis Romero <lpromero@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#483367}

TBR=sdefresne@chromium.org,lpromero@chromium.org,gambard@chromium.org

Change-Id: I8799d7bb3a02154f1a35dd07532aaeed0a8e50b9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  737110 
Reviewed-on: https://chromium-review.googlesource.com/556130
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483387}
[modify] https://crrev.com/b4519a14f8ecb2b6a0ca5131203741e38ec10796/ios/chrome/browser/content_suggestions/BUILD.gn
[delete] https://crrev.com/250c1aa680065119692b5c4bc81ddd36da12679f/ios/chrome/browser/content_suggestions/content_suggestions_alert_commands.h
[delete] https://crrev.com/250c1aa680065119692b5c4bc81ddd36da12679f/ios/chrome/browser/content_suggestions/content_suggestions_alert_egtest.mm
[delete] https://crrev.com/250c1aa680065119692b5c4bc81ddd36da12679f/ios/chrome/browser/content_suggestions/content_suggestions_alert_factory.h
[delete] https://crrev.com/250c1aa680065119692b5c4bc81ddd36da12679f/ios/chrome/browser/content_suggestions/content_suggestions_alert_factory.mm
[modify] https://crrev.com/b4519a14f8ecb2b6a0ca5131203741e38ec10796/ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm
[modify] https://crrev.com/b4519a14f8ecb2b6a0ca5131203741e38ec10796/ios/chrome/test/earl_grey/BUILD.gn

Project Member

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

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

commit 40563df09726bf21af749ed480bfd05f47381793
Author: gambard <gambard@chromium.org>
Date: Fri Jun 30 09:03:52 2017

Reland of "Move the ContentSuggestions alerts to an object"

The ContentSuggestionsCoordinator is doing too many things.
This CL mitigates this by moving the creations of the alerts
to another class.

BUG= 737110 

Change-Id: Ifafb67f33979f31fb8bd755d1a3f30485f0c875f
Reviewed-on: https://chromium-review.googlesource.com/557800
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483660}
[modify] https://crrev.com/40563df09726bf21af749ed480bfd05f47381793/ios/chrome/browser/content_suggestions/BUILD.gn
[add] https://crrev.com/40563df09726bf21af749ed480bfd05f47381793/ios/chrome/browser/content_suggestions/content_suggestions_alert_commands.h
[add] https://crrev.com/40563df09726bf21af749ed480bfd05f47381793/ios/chrome/browser/content_suggestions/content_suggestions_alert_egtest.mm
[add] https://crrev.com/40563df09726bf21af749ed480bfd05f47381793/ios/chrome/browser/content_suggestions/content_suggestions_alert_factory.h
[add] https://crrev.com/40563df09726bf21af749ed480bfd05f47381793/ios/chrome/browser/content_suggestions/content_suggestions_alert_factory.mm
[modify] https://crrev.com/40563df09726bf21af749ed480bfd05f47381793/ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm
[modify] https://crrev.com/40563df09726bf21af749ed480bfd05f47381793/ios/chrome/test/earl_grey/BUILD.gn

Comment 4 by fi...@chromium.org, Jul 21 2017

Labels: zine-triaged
Labels: -Type-Feature Type-Task
Status: Fixed (was: Assigned)

Sign in to add a comment