Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
912653005:
bookmarks: Remove url_database.h entry from DEPS list. (Closed)
|
Created:
5 years, 10 months ago by tfarina Modified:
5 years, 10 months ago CC:
blundell, browser-components-watch_chromium.org, chromium-reviews, noyau+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbookmarks: Remove DEPS file now that componentization is almost done.
Since the componentization of bookmarks is mostly done, we don't need the rules forbidding usage of //chrome/browser anymore, as the files still in
//chrome/browser/bookmarks won't be moved in the component (factories,
client, etc, all belongs to the embedder, not the component).
BUG=383597
TEST=checkdeps
R=sky@chromium.org,sdefresne@chromium.org
Committed: https://crrev.com/de85300a1986e3ce43f89a56b08dbd5e2049c041
Cr-Commit-Position: refs/heads/master@{#316201}
Patch Set 1 #
Total comments: 2
Patch Set 2 : rm DEPS file #Messages
Total messages: 17 (2 generated)
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
https://codereview.chromium.org/912653005/diff/1/chrome/browser/bookmarks/DEPS File chrome/browser/bookmarks/DEPS (right): https://codereview.chromium.org/912653005/diff/1/chrome/browser/bookmarks/DEP... chrome/browser/bookmarks/DEPS:2: # Bookmarks is being made into a component (it will end up at Since the componentization of bookmarks is mostly done, I think we can instead remove the rule forbidding usage of //chrome/browser as the files still in //chrome/browser/bookmarks won't be moved in the component ever (factories, client, ... belongs to the embedder, not the component). This means we can also get rid of the + and ! exceptions rules and all the comments.
https://codereview.chromium.org/912653005/diff/1/chrome/browser/bookmarks/DEPS File chrome/browser/bookmarks/DEPS (right): https://codereview.chromium.org/912653005/diff/1/chrome/browser/bookmarks/DEP... chrome/browser/bookmarks/DEPS:2: # Bookmarks is being made into a component (it will end up at On 2015/02/10 13:51:20, sdefresne wrote: > Since the componentization of bookmarks is mostly done, I think we can instead > remove the rule forbidding usage of //chrome/browser as the files still in > //chrome/browser/bookmarks won't be moved in the component ever (factories, > client, ... belongs to the embedder, not the component). > > This means we can also get rid of the + and ! exceptions rules and all the > comments. I'm not sure what you are proposing exactly. To remove the DEPS altogether?
On 2015/02/10 at 13:55:19, tfarina wrote: > https://codereview.chromium.org/912653005/diff/1/chrome/browser/bookmarks/DEPS > File chrome/browser/bookmarks/DEPS (right): > > https://codereview.chromium.org/912653005/diff/1/chrome/browser/bookmarks/DEP... > chrome/browser/bookmarks/DEPS:2: # Bookmarks is being made into a component (it will end up at > On 2015/02/10 13:51:20, sdefresne wrote: > > Since the componentization of bookmarks is mostly done, I think we can instead > > remove the rule forbidding usage of //chrome/browser as the files still in > > //chrome/browser/bookmarks won't be moved in the component ever (factories, > > client, ... belongs to the embedder, not the component). > > > > This means we can also get rid of the + and ! exceptions rules and all the > > comments. > > I'm not sure what you are proposing exactly. To remove the DEPS altogether? Yes, that's my suggestion as none of the file in chrome/browser/history are going to be componentized.
On 2015/02/10 14:48:52, sdefresne wrote: > On 2015/02/10 at 13:55:19, tfarina wrote: > > https://codereview.chromium.org/912653005/diff/1/chrome/browser/bookmarks/DEPS > > File chrome/browser/bookmarks/DEPS (right): > > > > > https://codereview.chromium.org/912653005/diff/1/chrome/browser/bookmarks/DEP... > > chrome/browser/bookmarks/DEPS:2: # Bookmarks is being made into a component > (it will end up at > > On 2015/02/10 13:51:20, sdefresne wrote: > > > Since the componentization of bookmarks is mostly done, I think we can > instead > > > remove the rule forbidding usage of //chrome/browser as the files still in > > > //chrome/browser/bookmarks won't be moved in the component ever (factories, > > > client, ... belongs to the embedder, not the component). > > > > > > This means we can also get rid of the + and ! exceptions rules and all the > > > comments. > > > > I'm not sure what you are proposing exactly. To remove the DEPS altogether? > > Yes, that's my suggestion as none of the file in chrome/browser/history are > going to be componentized. In chrome/browser/bookmarks you mean?
On 2015/02/10 at 14:56:08, tfarina wrote: > On 2015/02/10 14:48:52, sdefresne wrote: > > On 2015/02/10 at 13:55:19, tfarina wrote: > > > https://codereview.chromium.org/912653005/diff/1/chrome/browser/bookmarks/DEPS > > > File chrome/browser/bookmarks/DEPS (right): > > > > > > > > https://codereview.chromium.org/912653005/diff/1/chrome/browser/bookmarks/DEP... > > > chrome/browser/bookmarks/DEPS:2: # Bookmarks is being made into a component > > (it will end up at > > > On 2015/02/10 13:51:20, sdefresne wrote: > > > > Since the componentization of bookmarks is mostly done, I think we can > > instead > > > > remove the rule forbidding usage of //chrome/browser as the files still in > > > > //chrome/browser/bookmarks won't be moved in the component ever (factories, > > > > client, ... belongs to the embedder, not the component). > > > > > > > > This means we can also get rid of the + and ! exceptions rules and all the > > > > comments. > > > > > > I'm not sure what you are proposing exactly. To remove the DEPS altogether? > > > > Yes, that's my suggestion as none of the file in chrome/browser/history are > > going to be componentized. > > In chrome/browser/bookmarks you mean? Yes, in chrome/browser/bookmarks. They are all specific to Chrome.
Sylvain, I removed the DEPS file. Rietveld is confusingly showing M instead of D, but the Raw diff shows it has been deleted correctly. PTAL!
Could you update the CL description? lgtm
Yep. CL description updated.
Scott, I need your approval as well.
LGTM
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/912653005/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/de85300a1986e3ce43f89a56b08dbd5e2049c041 Cr-Commit-Position: refs/heads/master@{#316201}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/920403002/ by kaliamoorthi@chromium.org. The reason for reverting is: Seem to cause a failure related to bookmark dependencies.. |
