Move ReadingList model to components/reading_list/core

Move ReadingList model to components/reading_list/core

- Needed to rename ERROR enums to DISTILLATION_ERROR as ERROR is defined on Windows.
- Needed to fix FILE_PATH_LITERAL for Windows.
- Needed to return a default value when returning from an switch for Android.

BUG=704105
Review-Url: https://codereview.chromium.org/2763233003
Cr-Commit-Position: refs/heads/master@{#459067}
Committed: https://chromium.googlesource.com/chromium/src/+/52b6cd6ecd26d8978e61fa708dd857c2909e49be

[Olivier, 2017-03-22 13:34:37]

Description was changed from

==========
Move ReadingList model to components/reading_list/core

BUG=
==========

to

==========
Move ReadingList model to components/reading_list/core

BUG=704105
==========

[Olivier, 2017-03-22 13:34:53]

olivierrobin@chromium.org changed reviewers:
+ gambard@chromium.org

[Olivier, 2017-03-22 13:34:53]

Tadaaaam!

[Olivier, 2017-03-22 13:35:02]

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

[commit-bot: I haz the power, 2017-03-22 13:35:31]

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

[commit-bot: I haz the power, 2017-03-22 13:44:26]

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

[commit-bot: I haz the power, 2017-03-22 13:44:27]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[gambard, 2017-03-22 13:56:22]

Thanks!
Maybe it would be better to have the flags in a different folder? I am not sure
and I don't feel strongly about this issue.
lgtm as is

https://codereview.chromium.org/2763233003/diff/1/components/reading_list/ios...
File components/reading_list/ios/BUILD.gn (right):

https://codereview.chromium.org/2763233003/diff/1/components/reading_list/ios...
components/reading_list/ios/BUILD.gn:14: "//components/reading_list/core:flags",
I am not sure you need this.

[Olivier, 2017-03-22 14:09:07]

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

[commit-bot: I haz the power, 2017-03-22 14:09:22]

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

[Olivier, 2017-03-22 14:09:24]

THank

https://codereview.chromium.org/2763233003/diff/1/components/reading_list/ios...
File components/reading_list/ios/BUILD.gn (right):

https://codereview.chromium.org/2763233003/diff/1/components/reading_list/ios...
components/reading_list/ios/BUILD.gn:14: "//components/reading_list/core:flags",
On 2017/03/22 13:56:22, gambard wrote:
> I am not sure you need this.

Done.

[Olivier, 2017-03-22 14:09:27]

THanks

[commit-bot: I haz the power, 2017-03-22 14:35:27]

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

[commit-bot: I haz the power, 2017-03-22 14:35:28]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Olivier, 2017-03-22 14:48:43]

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

[commit-bot: I haz the power, 2017-03-22 14:49:04]

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

[commit-bot: I haz the power, 2017-03-22 15:24:03]

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

[commit-bot: I haz the power, 2017-03-22 15:24:04]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Olivier, 2017-03-22 15:48:53]

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

[commit-bot: I haz the power, 2017-03-22 15:49:31]

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

[Olivier, 2017-03-22 16:06:46]

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

[commit-bot: I haz the power, 2017-03-22 16:07:13]

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

[commit-bot: I haz the power, 2017-03-22 16:44:50]

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

[commit-bot: I haz the power, 2017-03-22 16:44:51]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Olivier, 2017-03-22 17:00:50]

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

[commit-bot: I haz the power, 2017-03-22 17:01:17]

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

[Olivier, 2017-03-22 17:23:36]

Description was changed from

==========
Move ReadingList model to components/reading_list/core

BUG=704105
==========

to

==========
Move ReadingList model to components/reading_list/core

- Needed to rename ERROR enums to DISTILLATION_ERROR as ERROR is defined on
Windows.
- Needed to fix FILE_PATH_LITERAL for Windows.
- Needed to return a default value when returning from an switch for Android.

BUG=704105
==========

[commit-bot: I haz the power, 2017-03-22 17:35:38]

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

[commit-bot: I haz the power, 2017-03-22 17:35:39]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Olivier, 2017-03-22 17:42:24]

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

[commit-bot: I haz the power, 2017-03-22 17:43:05]

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

[Olivier, 2017-03-22 18:22:55]

olivierrobin@chromium.org changed reviewers:
+ sdefresne@chromium.org

[commit-bot: I haz the power, 2017-03-22 18:58:11]

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

[commit-bot: I haz the power, 2017-03-22 18:58:12]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Olivier, 2017-03-23 08:53:49]

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

[commit-bot: I haz the power, 2017-03-23 08:54:09]

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

[commit-bot: I haz the power, 2017-03-23 10:00:01]

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

[commit-bot: I haz the power, 2017-03-23 10:00:02]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Olivier, 2017-03-23 10:12:44]

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

[commit-bot: I haz the power, 2017-03-23 10:13:00]

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

[sdefresne, 2017-03-23 10:27:13]

Do you want me to review this CL? I've been added as reviewer but the email was
empty ;-)

[Olivier, 2017-03-23 10:27:48]

On 2017/03/23 10:27:13, sdefresne wrote:
> Do you want me to review this CL? I've been added as reviewer but the email
was
> empty ;-)

Yes, PTAL, thanks.

[sdefresne, 2017-03-23 10:57:06]

lgtm

https://codereview.chromium.org/2763233003/diff/160001/components/reading_lis...
File components/reading_list/core/offline_url_utils.cc (right):

https://codereview.chromium.org/2763233003/diff/160001/components/reading_lis...
components/reading_list/core/offline_url_utils.cc:12: const
base::FilePath::CharType kOfflineDirectory[] =
optional: it is quite common to define a macro FPL in .cc file and to use it to
declare file path constants on a single line.


#define FPL FILE_PATH_LITERAL
namespace {
const base::FilePath::CharType kOfflineDirectory[] = FPL("Offline");
...

https://codereview.chromium.org/2763233003/diff/160001/components/reading_lis...
File components/reading_list/ios/reading_list_model_bridge_observer.h (right):

https://codereview.chromium.org/2763233003/diff/160001/components/reading_lis...
components/reading_list/ios/reading_list_model_bridge_observer.h:15: @protocol
ReadingListModelBridgeObserver<NSObject>
optional, follow-up: I think it should be called ReadingListModelObserving.

https://codereview.chromium.org/2763233003/diff/160001/ios/chrome/browser/sha...
File ios/chrome/browser/share_extension/share_extension_service.mm (right):

https://codereview.chromium.org/2763233003/diff/160001/ios/chrome/browser/sha...
ios/chrome/browser/share_extension/share_extension_service.mm:6: #include
"base/mac/scoped_nsobject.h"
Please add a blank line before #include "base/mac/scoped_nsobject.h".

[commit-bot: I haz the power, 2017-03-23 11:30:13]

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

[commit-bot: I haz the power, 2017-03-23 11:30:14]

Dry run: This issue passed the CQ dry run.

[Olivier, 2017-03-23 12:27:37]

Thanks

I will rename the class in another CL.

https://codereview.chromium.org/2763233003/diff/160001/components/reading_lis...
File components/reading_list/core/offline_url_utils.cc (right):

https://codereview.chromium.org/2763233003/diff/160001/components/reading_lis...
components/reading_list/core/offline_url_utils.cc:12: const
base::FilePath::CharType kOfflineDirectory[] =
On 2017/03/23 10:57:06, sdefresne wrote:
> optional: it is quite common to define a macro FPL in .cc file and to use it
to
> declare file path constants on a single line.
> 
> 
> #define FPL FILE_PATH_LITERAL
> namespace {
> const base::FilePath::CharType kOfflineDirectory[] = FPL("Offline");
> ...

Thanks.
I think I prefer this format

https://codereview.chromium.org/2763233003/diff/160001/components/reading_lis...
File components/reading_list/ios/reading_list_model_bridge_observer.h (right):

https://codereview.chromium.org/2763233003/diff/160001/components/reading_lis...
components/reading_list/ios/reading_list_model_bridge_observer.h:15: @protocol
ReadingListModelBridgeObserver<NSObject>
On 2017/03/23 10:57:06, sdefresne wrote:
> optional, follow-up: I think it should be called ReadingListModelObserving.

Thanks. I will change that in a next CL.
What is the difference between ...Observer and ...Observing pattern?

https://codereview.chromium.org/2763233003/diff/160001/ios/chrome/browser/sha...
File ios/chrome/browser/share_extension/share_extension_service.mm (right):

https://codereview.chromium.org/2763233003/diff/160001/ios/chrome/browser/sha...
ios/chrome/browser/share_extension/share_extension_service.mm:6: #include
"base/mac/scoped_nsobject.h"
On 2017/03/23 10:57:06, sdefresne wrote:
> Please add a blank line before #include "base/mac/scoped_nsobject.h".

Done.

[Olivier, 2017-03-23 12:27:46]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-03-23 12:27:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
gambard@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2763233003/#ps180001
(title: "feedback")

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

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

[commit-bot: I haz the power, 2017-03-23 13:56:01]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490272066971760,
"parent_rev": "f72cfd9f7717eaa5341d7223b01c427cf19bca08", "commit_rev":
"52b6cd6ecd26d8978e61fa708dd857c2909e49be"}

[commit-bot: I haz the power, 2017-03-23 13:56:46]

Description was changed from

==========
Move ReadingList model to components/reading_list/core

- Needed to rename ERROR enums to DISTILLATION_ERROR as ERROR is defined on
Windows.
- Needed to fix FILE_PATH_LITERAL for Windows.
- Needed to return a default value when returning from an switch for Android.

BUG=704105
==========

to

==========
Move ReadingList model to components/reading_list/core

- Needed to rename ERROR enums to DISTILLATION_ERROR as ERROR is defined on
Windows.
- Needed to fix FILE_PATH_LITERAL for Windows.
- Needed to return a default value when returning from an switch for Android.

BUG=704105

Review-Url: https://codereview.chromium.org/2763233003
Cr-Commit-Position: refs/heads/master@{#459067}
Committed:
https://chromium.googlesource.com/chromium/src/+/52b6cd6ecd26d8978e61fa708dd8...
==========

[commit-bot: I haz the power, 2017-03-23 13:56:48]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/52b6cd6ecd26d8978e61fa708dd8...