Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2014733003:
Removing parsing of text from pasteboard. (Closed)
DescriptionRemoving parsing of text from pasteboard.
This patch modifies logic for drag and dropping text that can be parsed
as URL. Previously pasteboard would unconditionally answer that it
contains URL. For some cases it is better to process such text using
PasteAndGo logic.
R=mark@chromium.org, pkasting@chromium.org
BUG=610620
Committed: https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957
Cr-Commit-Position: refs/heads/master@{#397689}
Patch Set 1 : mac: stop parsing text as url. #
Total comments: 2
Patch Set 2 : splitting calls in two variations, to discass #
Total comments: 7
Patch Set 3 : Replacing overloads with bool parametr, review fixes #Patch Set 4 : starting to modify os echange data. Now it compiles on mac. #Patch Set 5 : intial windows patch, compiles #Patch Set 6 : moving url parsing up the hierarchy #
Total comments: 1
Patch Set 7 : Reverting everything, except mac. Review mac. #
Total comments: 2
Patch Set 8 : Review, round n #
Total comments: 2
Patch Set 9 : Review, round n + 1 #
Total comments: 1
Patch Set 10 : Review, round n + 2 #Patch Set 11 : Compilation fix #Messages
Total messages: 66 (25 generated)
The CQ bit was checked by dyaroshev@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2014733003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2014733003/1
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2014733003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2014733003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2014733003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2014733003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
I've updated all calls, that get url data on mac, so it's easier to discuss, which ones should be used where. Actually, it would be great, if we could just leave one version. But, right now, I am not sure, that we can.
Description was changed from ========== Removing parsing of text from pasteboard. When text get's dragged and dropped, it should be processed through PasteAndGo logic. Instead, what used to happen, text was attempted to be parsed as URL, and if it successed, pasteboard returned, that url was dragged. R=mark@chromium.org, pkasting@chromium.org BUG=610620 ========== to ========== Removing parsing of text from pasteboard. When text get's dragged and dropped, it should be processed through PasteAndGo logic. Instead, what used to happen, text was attempted to be parsed as URL, and if it successed, pasteboard returned, that url was dragged. R=mark@chromium.org, pkasting@chromium.org BUG=610620 ==========
dyaroshev@yandex-team.ru changed reviewers: + avi@chromium.org
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
The CQ bit was unchecked by dyaroshev@yandex-team.ru
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2014733003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2014733003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/2014733003/diff/1/chrome/browser/ui/cocoa/url... File chrome/browser/ui/cocoa/url_drop_target_unittest.mm (right): https://codereview.chromium.org/2014733003/diff/1/chrome/browser/ui/cocoa/url... chrome/browser/ui/cocoa/url_drop_target_unittest.mm:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. CL description: > When text get's dragged and dropped, it should be processed through get's → gets https://codereview.chromium.org/2014733003/diff/1/chrome/browser/ui/cocoa/url... chrome/browser/ui/cocoa/url_drop_target_unittest.mm:2: // Use of this source code is governed by a BSD-style license that can be CL description: > PasteAndGo logic. Instead, what used to happen, text was attempted > to be parsed as URL, and if it successed, pasteboard returned, > that url was dragged. This second sentence needs to be revised. https://codereview.chromium.org/2014733003/diff/20001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/2014733003/diff/20001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:1325: 'browser/ui/cocoa/url_drop_target_unittest.mm', There will be a BUILD file for gn that you should update too. https://codereview.chromium.org/2014733003/diff/20001/third_party/mozilla/NSP... File third_party/mozilla/NSPasteboard+Utils.mm (right): https://codereview.chromium.org/2014733003/diff/20001/third_party/mozilla/NSP... third_party/mozilla/NSPasteboard+Utils.mm:240: - (void)getURLsFromTypeOrText:(NSArray**)outUrls GetURLsOrTextFromType https://codereview.chromium.org/2014733003/diff/20001/third_party/mozilla/NSP... third_party/mozilla/NSPasteboard+Utils.mm:241: andTitles:(NSArray**)outTitles align colons https://codereview.chromium.org/2014733003/diff/20001/third_party/mozilla/NSP... third_party/mozilla/NSPasteboard+Utils.mm:246: andTitles:outTitles align colons https://codereview.chromium.org/2014733003/diff/20001/third_party/mozilla/REA... File third_party/mozilla/README.chromium (right): https://codereview.chromium.org/2014733003/diff/20001/third_party/mozilla/REA... third_party/mozilla/README.chromium:26: -[NSPasteboard:containsURLData] and[NSPasteboard getURLs:andTitles:] were “and -[NSPasteboard” https://codereview.chromium.org/2014733003/diff/20001/third_party/mozilla/REA... third_party/mozilla/README.chromium:27: splitted in two, depending on if user copy pasted text, do we wanna try to split in two
avi@chromium.org changed reviewers: + erikchen@chromium.org
Erik has been doing lots of drag work; he's the expert.
https://codereview.chromium.org/2014733003/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/url_drop_target.mm (right): https://codereview.chromium.org/2014733003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/url_drop_target.mm:87: if ([pboard containsURLDataInType]) { I don't think this is the right approach. If a user drag and drops a a plain text URL stored in a NSStringPboardType from another application into Chrome, it should still be interpreted as a URL. I'm surprised that your newly added test UrlDropControllerTest.DragAndDropURL passes. It is because you're using ui::ClipboardUtil::PasteboardItemFromUrls, which internally uses NSPasteboardTypeString, which Pasteboard is smart with, and converts into a URL type? It looks like the problem is that the methods """ - (void) getURLs:(NSArray**)outUrls andTitles:(NSArray**)outTitles convertingFilenames:(BOOL)convertFilenames; - (BOOL)containsURLData; """ are trying to perform some URI cleanup with a hacked together method cleanedStringWithPasteboardString: which trims whitespace, and does some other nonsensical stuff. I think the right solution is: 1. Expand the function ClipboardUtil::URLsAndTitlesFromPasteboard in ui/base/clipboard/clipboard_util_mac.mm to handle more Pboard URIs, to better match the behavior of: """ - (void) getURLs:(NSArray**)outUrls andTitles:(NSArray**)outTitles convertingFilenames:(BOOL)convertFilenames; """ Use functions from GURL to determine whether the url is valid. Note that this will require a new dependency from ui/base/clipboard/DEPS onto url/. I think this is okay, but check with avi@ and an OWNER of url/. 2. Replace the call here to [pboard containsURLData] with an appropriate call to ClipboardUtil::URLsAndTitlesFromPasteboard.
On 2016/05/26 21:01:18, erikchen wrote: > Use functions from GURL to determine whether the url is valid. If we need to take a string of plain text, which may or may not be a URL, and decide whether to treat it as a URL, we can't use GURL to do this, as it will have both false positives and false negatives w.r.t. user intent. We need to use the same machinery used by Paste-And-Search/Paste-And-Go (which is built atop the AutocompleteClassifier). I haven't reviewed this CL yet, but one issue that led to this was that at least some callers were trying to use the clipboard machinery to get the precise type of object on the clipboard, and then for text types, running that through the paste-and-xxx functions to determine whether to treat them as URLs. This works fairly well but has the downside that every caller of clipboard functions that wants this behavior has to do it. The alternative is to hoist this handling into the clipboard code itself, which has the downside of making this code depend on that functionality and making it impossible for callers to get at the true underlying clipboard object types; callers that want to distinguish a true URL from the plain text of a URL won't be able to do so. Don't know if anyone cares about that.
On 2016/05/26 21:12:36, Peter Kasting wrote: > I haven't reviewed this CL yet, I just realized this CL is Mac-specific, so I won't be reviewing it anyway. :P The rest of my comment should still apply.
Description was changed from ========== Removing parsing of text from pasteboard. When text get's dragged and dropped, it should be processed through PasteAndGo logic. Instead, what used to happen, text was attempted to be parsed as URL, and if it successed, pasteboard returned, that url was dragged. R=mark@chromium.org, pkasting@chromium.org BUG=610620 ========== to ========== Removing parsing of text from pasteboard. This patch modifies logic for drag and dropping text that can be parsed as URL. Previously pasteboard, would unconditionally answer, that it contains URL. For some cases, it is better to process such text using PasteAndGo logic. R=mark@chromium.org, pkasting@chromium.org BUG=610620 ==========
Description was changed from ========== Removing parsing of text from pasteboard. This patch modifies logic for drag and dropping text that can be parsed as URL. Previously pasteboard, would unconditionally answer, that it contains URL. For some cases, it is better to process such text using PasteAndGo logic. R=mark@chromium.org, pkasting@chromium.org BUG=610620 ========== to ========== Removing parsing of text from pasteboard. This patch modifies logic for drag and dropping text that can be parsed as URL. Previously pasteboard would unconditionally answer that it contains URL. For some cases it is better to process such text using PasteAndGo logic. R=mark@chromium.org, pkasting@chromium.org BUG=610620 ==========
On 2016/05/26 20:21:51, Mark Mentovai wrote: > https://codereview.chromium.org/2014733003/diff/1/chrome/browser/ui/cocoa/url... > chrome/browser/ui/cocoa/url_drop_target_unittest.mm:1: // Copyright (c) 2016 The > Chromium Authors. All rights reserved. > CL description: > > > When text get's dragged and dropped, it should be processed through > > get's → gets > > https://codereview.chromium.org/2014733003/diff/1/chrome/browser/ui/cocoa/url... > chrome/browser/ui/cocoa/url_drop_target_unittest.mm:2: // Use of this source > code is governed by a BSD-style license that can be > CL description: > > > PasteAndGo logic. Instead, what used to happen, text was attempted > > to be parsed as URL, and if it successed, pasteboard returned, > > that url was dragged. > > This second sentence needs to be revised. I did something more reasonable. > https://codereview.chromium.org/2014733003/diff/20001/chrome/chrome_tests_uni... > chrome/chrome_tests_unit.gypi:1325: > 'browser/ui/cocoa/url_drop_target_unittest.mm', > There will be a BUILD file for gn that you should update too. There is no BUILD.gn for similar tests yet. Adding one is quite a task. > https://codereview.chromium.org/2014733003/diff/20001/third_party/mozilla/NSP... > third_party/mozilla/NSPasteboard+Utils.mm:240: - > (void)getURLsFromTypeOrText:(NSArray**)outUrls > GetURLsOrTextFromType > This is third party. Should I still rename a function? > https://codereview.chromium.org/2014733003/diff/20001/third_party/mozilla/NSP... > third_party/mozilla/NSPasteboard+Utils.mm:241: andTitles:(NSArray**)outTitles > align colons > > https://codereview.chromium.org/2014733003/diff/20001/third_party/mozilla/NSP... > third_party/mozilla/NSPasteboard+Utils.mm:246: andTitles:outTitles > align colons > I ran "git cl format", and then reduced diff in third party. There are couple of mistakes, I'll fix those in next patches. Other than that, seems good? > https://codereview.chromium.org/2014733003/diff/20001/third_party/mozilla/REA... > third_party/mozilla/README.chromium:26: -[NSPasteboard:containsURLData] > and[NSPasteboard getURLs:andTitles:] were > “and -[NSPasteboard” > > https://codereview.chromium.org/2014733003/diff/20001/third_party/mozilla/REA... > third_party/mozilla/README.chromium:27: splitted in two, depending on if user > copy pasted text, do we wanna try to > split in two Redid help
On 2016/05/26 21:01:18, erikchen wrote: > https://codereview.chromium.org/2014733003/diff/20001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/url_drop_target.mm:87: if ([pboard > containsURLDataInType]) { > I don't think this is the right approach. If a user drag and drops a a plain > text URL stored in a NSStringPboardType from another application into Chrome, it > should still be interpreted as a URL. > For the case I'm working on, dropText will be called, which will call AutocompleteClassify. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... If text is most likely to be a URL, Autocomplete will figure it out, and navigation will be occur. > I'm surprised that your newly added test UrlDropControllerTest.DragAndDropURL > passes. It is because you're using ui::ClipboardUtil::PasteboardItemFromUrls, > which internally uses NSPasteboardTypeString, which Pasteboard is smart with, > and converts into a URL type? > If I understood you correctly, yes. PasteboardItemFromUrls sets types: https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/clipboard/... NSPasteboard+Utils checks for those types: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mozill... If I'm correct, it's the same as if I would drag smth, that is already considered to by a url. It even looks differently from dragging text. > It looks like the problem is that the methods > """ > - (void) getURLs:(NSArray**)outUrls > andTitles:(NSArray**)outTitles > convertingFilenames:(BOOL)convertFilenames; > - (BOOL)containsURLData; > """ > are trying to perform some URI cleanup with a hacked together method > cleanedStringWithPasteboardString: which trims whitespace, and does some other > nonsensical stuff. > > I think the right solution is: > 1. Expand the function ClipboardUtil::URLsAndTitlesFromPasteboard in > ui/base/clipboard/clipboard_util_mac.mm to handle more Pboard URIs, to better > match the behavior of: > """ > - (void) getURLs:(NSArray**)outUrls > andTitles:(NSArray**)outTitles > convertingFilenames:(BOOL)convertFilenames; > """ > > Use functions from GURL to determine whether the url is valid. Note that this > will require a new dependency from ui/base/clipboard/DEPS onto url/. I think > this is okay, but check with avi@ and an OWNER of url/. > > 2. Replace the call here to [pboard containsURLData] with an appropriate call to > ClipboardUtil::URLsAndTitlesFromPasteboard. I'm a bit confused. Are you suggesting to: 1) not modifing NSPasteboard+Utils 2) in url_drop_target.mm use ClipboardUtil::URLsAndTitlesFromPasteboard, with some modifications, instead?
> If I understood you correctly, yes. > PasteboardItemFromUrls sets types: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/clipboard/... > NSPasteboard+Utils checks for those types: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mozill... > > If I'm correct, it's the same as if I would drag smth, that is already > considered to by a url. It even looks differently from dragging text. Then your test isn't useful. The two cases you need to be able to distinguish are: 1. A Clipboard with NSStringPboardType/NSPasteboardTypeString with text that contains a real URL 2. A Clipboard with NSStringPboardType/NSPasteboardTypeString with text that contains other text. > > > It looks like the problem is that the methods > > """ > > - (void) getURLs:(NSArray**)outUrls > > andTitles:(NSArray**)outTitles > > convertingFilenames:(BOOL)convertFilenames; > > - (BOOL)containsURLData; > > """ > > are trying to perform some URI cleanup with a hacked together method > > cleanedStringWithPasteboardString: which trims whitespace, and does some other > > nonsensical stuff. > > > > I think the right solution is: > > 1. Expand the function ClipboardUtil::URLsAndTitlesFromPasteboard in > > ui/base/clipboard/clipboard_util_mac.mm to handle more Pboard URIs, to better > > match the behavior of: > > """ > > - (void) getURLs:(NSArray**)outUrls > > andTitles:(NSArray**)outTitles > > convertingFilenames:(BOOL)convertFilenames; > > """ > > > > Use functions from GURL to determine whether the url is valid. Note that this > > will require a new dependency from ui/base/clipboard/DEPS onto url/. I think > > this is okay, but check with avi@ and an OWNER of url/. > > > > 2. Replace the call here to [pboard containsURLData] with an appropriate call > to > > ClipboardUtil::URLsAndTitlesFromPasteboard. > > I'm a bit confused. Are you suggesting to: > 1) not modifing NSPasteboard+Utils > 2) in url_drop_target.mm use ClipboardUtil::URLsAndTitlesFromPasteboard, with > some modifications, instead? Your current code does not get us closer to a correct, clean, sane solution. Doing so would require refactoring some logic out of "Paste-And-Search/Paste-And-Go", which may be difficult. I'm pretty sure your current code is wrong, since it prevents a Clipboard drag with contents: NSStringPboardType/"http://cnn.com" from being recognized as a URL. You should make a copy of the test """ 129 TEST_F(UrlDropControllerTest, DragAndDropURL) { 130 NSArray* urls = [NSArray arrayWithObject:@"http://abc.xyz"]; 131 NSArray* titles = [NSArray arrayWithObject:@"abc"]; 132 133 [[ControllerMock() expect] dropURLs:urls inView:View() at:kDropPoint]; 134 135 base::scoped_nsobject<NSPasteboardItem> item = 136 ui::ClipboardUtil::PasteboardItemFromUrls(urls, titles); 137 138 [View() performDragOperation:GetFakeDragInfoForPasteboardItem(item.get())]; 139 [ControllerMock() verify]; 140 } """ which instead makes a the NSPasteboardItem using """ base::scoped_nsobject<NSPasteboardItem> item = ui::ClipboardUtil::PasteboardItemFromString(@"http://cnn.com"); """ and confirm that dropURLs: is called instead of dropText:.
On 2016/05/27 17:16:31, erikchen wrote: > Then your test isn't useful. The two cases you need to be able to distinguish > are: > 1. A Clipboard with NSStringPboardType/NSPasteboardTypeString with text that > contains a real URL > 2. A Clipboard with NSStringPboardType/NSPasteboardTypeString with text that > contains other text. > This test is about a case of url with text. So, lets say, at this page: BUG=610620 - is a link. However, if you just gonna to process text, Autocomplete will rightfully send you to search. We need to check type of pasteboard. (it's a reaction to this comment: https://bugs.chromium.org/p/chromium/issues/detail?id=610620#c10) > Your current code does not get us closer to a correct, clean, sane solution. > Doing so would require refactoring some logic out of > "Paste-And-Search/Paste-And-Go", which may be difficult. > > I'm pretty sure your current code is wrong, since it prevents a Clipboard drag > with contents: > NSStringPboardType/"http://cnn.com" > from being recognized as a URL. > > You should make a copy of the test > """ > 129 TEST_F(UrlDropControllerTest, DragAndDropURL) { > 130 NSArray* urls = [NSArray arrayWithObject:@"http://abc.xyz"]; > 131 NSArray* titles = [NSArray arrayWithObject:@"abc"]; > 132 > 133 [[ControllerMock() expect] dropURLs:urls inView:View() at:kDropPoint]; > 134 > 135 base::scoped_nsobject<NSPasteboardItem> item = > 136 ui::ClipboardUtil::PasteboardItemFromUrls(urls, titles); > 137 > 138 [View() > performDragOperation:GetFakeDragInfoForPasteboardItem(item.get())]; > 139 [ControllerMock() verify]; > 140 } > """ > > which instead makes a the NSPasteboardItem using > """ > base::scoped_nsobject<NSPasteboardItem> item = > ui::ClipboardUtil::PasteboardItemFromString(@"http://cnn.com"); > """ > > and confirm that dropURLs: is called instead of dropText:. Currently, decision wether to consider URLish text is in other places. Let's take tab_stip. Right now, it uses an Autocomplete to determine on which url it'd like to navigate. However, let's take bookmarks. Because they can not do anything useful with text, we want to try really hard to parse text as url. Probably, the test you want me to right is the previous one: "query: query" by the standard, can be parsed as url. And I would argue that we should let controller decide how to interpret URLish text. So, in the case of: """ base::scoped_nsobject<NSPasteboardItem> item = ui::ClipboardUtil::PasteboardItemFromString(@"http://cnn.com"); """ We should dropText. However, I'm the first in line to agree that code is messy.
> Currently, decision wether to consider URLish text is in other places. > Let's take tab_stip. Right now, it uses an Autocomplete to determine on which > url it'd like to navigate. > > However, let's take bookmarks. Because they can not do anything useful with > text, we want to try really hard > to parse text as url. > > Probably, the test you want me to right is the previous one: "query: query" by > the standard, can be parsed as url. > And I would argue that we should let controller decide how to interpret URLish > text. Point taken. All the implementations of dropText: currently try to do some URLish conversions. I patched in your CL, and it doesn't break it any of the ways I thought it would break. > So, in the case of: > """ > base::scoped_nsobject<NSPasteboardItem> item = > ui::ClipboardUtil::PasteboardItemFromString(@"http://cnn.com"); > """ > We should dropText. > > However, I'm the first in line to agree that code is messy. Please add that test, since it makes the expected behavior clear to anyone else who comes looking. Wrap the CL description to 80 columns.
On 2016/05/27 17:56:37, erikchen wrote: > Please add that test, since it makes the expected behavior clear to anyone else > who comes looking. In later patches, I'm working on other platforms. > Wrap the CL description to 80 columns. Done
On 2016/05/27 00:05:42, Peter Kasting wrote: > On 2016/05/26 21:12:36, Peter Kasting wrote: > > I haven't reviewed this CL yet, > > I just realized this CL is Mac-specific, so I won't be reviewing it anyway. :P > I don't know how to break it to you.... =) Couple of things: 1) I've moved URL parsing logic up to os_exchange_data, because it's really the same on all the platforms. I didn't analyze it closely, but seems like it can be pulled of with "file" policy to. Seems like a good idea? 2) tab_strip is drag and drop done in a strange manner: There are two functions that deal with dragging urls: OnDragEntered: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... OnPerformDrop: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... Both of them are called from browser_view, but in one case PasteAndGo is done, and in other cases it's not: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... I would suggest to move URL processing logic from browser_view to tab_strip. However, there is a catch: to calculate PasteAndGoURL I need a profile. I see it in two places: browser_root_view or browser_tab_strip_controller(not in tab_strip_controller interface). So, as far as I can see, I can do few things: 1) not use existing view function for OnDragEntered, and use some alternative 2) modify TabStripController to give browser. Considering how many places it's used, and the fact, that I have to do smth with the fake one, might be quite difficult. 3) ask TabStripController for PasteAndGoURL. Seems quite out of place. 4) ask TabStripController to process the drop. What should I do?
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/2014733003/diff/100001/ui/base/dragdrop/os_ex... File ui/base/dragdrop/os_exchange_data.cc (right): https://codereview.chromium.org/2014733003/diff/100001/ui/base/dragdrop/os_ex... ui/base/dragdrop/os_exchange_data.cc:80: *url = std::move(test); FYI: GURL doesn't have a move constructor, so this doesn't actually do anything.
On 2016/05/28 19:37:55, dyaroshev wrote: > On 2016/05/27 17:56:37, erikchen wrote: > > Please add that test, since it makes the expected behavior clear to anyone > else > > who comes looking. > In later patches, I'm working on other platforms. > > > Wrap the CL description to 80 columns. > Done The CL description is longer than 80 columns. I don't see a new test. Also, the CL seems to have ballooned in size? Should this be 2 CLs? FWIW, I'm ready to LG patch set 2, once my comments have been addressed.
On 2016/05/30 00:25:32, erikchen wrote: > On 2016/05/28 19:37:55, dyaroshev wrote: > > On 2016/05/27 17:56:37, erikchen wrote: > > > Please add that test, since it makes the expected behavior clear to anyone > > else > > > who comes looking. > > In later patches, I'm working on other platforms. > > > > > Wrap the CL description to 80 columns. > > Done > > The CL description is longer than 80 columns. I kinda feel like we shouldn't ask people to waste their time doing this manually, as long as there is a reasonable summary in the first line, which is fully visible in all various git tools. If lines beyond that are too long, I sort of feel like people can read the CLs on the web, or git tools should suck less, or something. In the limit, if this is vitally important, the code review site should be automatically doing it or warning people or something. I don't think it's important enough for that to happen, and thus I don't think it's important enough that we should ask people to do this. It doesn't feel nearly as beneficial as e.g. the 80-column limit in source code. This is super trivial and I'm only speaking up because another reviewer brought up the exact same issue on a CL of mine a couple days ago.
Description was changed from ========== Removing parsing of text from pasteboard. This patch modifies logic for drag and dropping text that can be parsed as URL. Previously pasteboard would unconditionally answer that it contains URL. For some cases it is better to process such text using PasteAndGo logic. R=mark@chromium.org, pkasting@chromium.org BUG=610620 ========== to ========== Removing parsing of text from pasteboard. This patch modifies logic for drag and dropping text that can be parsed as URL. Previously pasteboard would unconditionally answer that it contains URL. For some cases it is better to process such text using PasteAndGo logic. R=mark@chromium.org, pkasting@chromium.org BUG=610620 ==========
On 2016/05/30 00:25:32, erikchen wrote: > The CL description is longer than 80 columns. My bad, for some reason I thought about fixing README to 80 columns. Sorry. Done. > I don't see a new test. I was working on Windows - therefor couldn't add a new test right away. Done. > Also, the CL seems to have ballooned in size? Should this be 2 CLs? FWIW, I'm ready to LG > patch set 2, once my comments have been addressed. I agree. Let's merge mac specific fixes here and do views in an other. One thing - I think that in this context bool parameter looks considerably better. Don't you agree?
lgtm % nits mark: It looks like the GN file actually pulls from the gypi file. yay... https://cs-staging.chromium.org/chromium/src/chrome/test/BUILD.gn?q=chrome_un... https://codereview.chromium.org/2014733003/diff/120001/third_party/mozilla/NS... File third_party/mozilla/NSPasteboard+Utils.mm (right): https://codereview.chromium.org/2014733003/diff/120001/third_party/mozilla/NS... third_party/mozilla/NSPasteboard+Utils.mm:268: if ( [types containsObject:kWebURLsWithTitlesPboardType] This formatting looks really odd to me. Are you copying the formatting from somewhere else in this file? If so, I don't see it. https://codereview.chromium.org/2014733003/diff/120001/third_party/mozilla/RE... File third_party/mozilla/README.chromium (right): https://codereview.chromium.org/2014733003/diff/120001/third_party/mozilla/RE... third_party/mozilla/README.chromium:27: modified to accept additional parametr :convertTextToURL whether to "modified to accept an additional parameter convertTextToURL:, which indicates whether the string contents of the pasteboard should be interpreted as a URL.
On 2016/05/31 17:15:51, erikchen wrote: > https://codereview.chromium.org/2014733003/diff/120001/third_party/mozilla/NS... > third_party/mozilla/NSPasteboard+Utils.mm:268: if ( [types > containsObject:kWebURLsWithTitlesPboardType] > This formatting looks really odd to me. Are you copying the formatting from > somewhere else in this file? If so, I don't see it. It was there originally - it's third party code. Do we want to format it to Chromium style, or do we want to keep diff to minimum, so it's easier to merge newer versions? Btw, I've changed these lines in a last patch, but I would revert them. Another question on the subject, because it's third party code, do I have to do smth else after patch is merged? Is https://www.chromium.org/developers/how-tos/get-the-code/working-with-nested-... relevant in this situation? > https://codereview.chromium.org/2014733003/diff/120001/third_party/mozilla/RE... > third_party/mozilla/README.chromium:27: modified to accept additional parametr > :convertTextToURL whether to > "modified to accept an additional parameter convertTextToURL:, which indicates > whether the string contents of the pasteboard should be interpreted as a URL. Done with a slight touch)
On 2016/06/01 18:09:03, dyaroshev wrote: > On 2016/05/31 17:15:51, erikchen wrote: > > > https://codereview.chromium.org/2014733003/diff/120001/third_party/mozilla/NS... > > third_party/mozilla/NSPasteboard+Utils.mm:268: if ( [types > > containsObject:kWebURLsWithTitlesPboardType] > > This formatting looks really odd to me. Are you copying the formatting from > > somewhere else in this file? If so, I don't see it. > > It was there originally - it's third party code. Do we want to format it to > Chromium style, or do we want to keep diff to minimum, so it's easier to merge > newer versions? > Btw, I've changed these lines in a last patch, but I would revert them. I think either is fine. Feel free to keep what you have in the latest patch set. > > Another question on the subject, because it's third party code, do I have to do > smth else after patch is merged? > Is > https://www.chromium.org/developers/how-tos/get-the-code/working-with-nested-... > relevant in this situation? This isn't a nested repo. > > > > https://codereview.chromium.org/2014733003/diff/120001/third_party/mozilla/RE... > > third_party/mozilla/README.chromium:27: modified to accept additional parametr > > :convertTextToURL whether to > > "modified to accept an additional parameter convertTextToURL:, which indicates > > whether the string contents of the pasteboard should be interpreted as a URL. > > Done with a slight touch) Still lgtm.
The CQ bit was checked by dyaroshev@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2014733003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2014733003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/05/26 20:23:49, Avi wrote: > Erik has been doing lots of drag work; he's the expert. Erik sad his l-g-t-m)) As far as I can see you are the owner of most files I changed. Can you please take a look?
https://codereview.chromium.org/2014733003/diff/140001/third_party/mozilla/NS... File third_party/mozilla/NSPasteboard+Utils.h (right): https://codereview.chromium.org/2014733003/diff/140001/third_party/mozilla/NS... third_party/mozilla/NSPasteboard+Utils.h:59: convertTextToURL:(BOOL)convertTextToURL; convertingTextToURL to match the previous selector section. https://codereview.chromium.org/2014733003/diff/140001/third_party/mozilla/NS... third_party/mozilla/NSPasteboard+Utils.h:60: - (BOOL) containsURLData:(BOOL)convertTextToURL; This doesn't work. Your code acknowledges that by having a comment everywhere: [pasteboard containsURLData:/*convertTextToURL*/ YES] which is awkward and bad. You need to rename this method so that the call is self-documenting. If I had to come up with something, I'd say - (BOOL) containsURLDataConvertingTextToURL:(BOOL)convertingTextToURL so that calls would be: [pasteboard containsURLDataConvertingTextToURL:YES] I'm open to a better name (that is the best I could come up with in two minutes) but what you have now isn't good.
On 2016/06/02 18:08:02, Avi wrote: > convertTextToURL:(BOOL)convertTextToURL; > convertingTextToURL to match the previous selector section. Done. > [pasteboard containsURLDataConvertingTextToURL:YES] > > I'm open to a better name (that is the best I could come up with in two minutes) > but what you have now isn't good. Done.
LGTM with minor comment tweak. https://codereview.chromium.org/2014733003/diff/160001/third_party/mozilla/RE... File third_party/mozilla/README.chromium (right): https://codereview.chromium.org/2014733003/diff/160001/third_party/mozilla/RE... third_party/mozilla/README.chromium:27: modified to accept an additional parameter convertTextToURL:, which indicates convertingTextToURL:
On 2016/06/02 19:12:56, Avi wrote: > https://codereview.chromium.org/2014733003/diff/160001/third_party/mozilla/RE... > third_party/mozilla/README.chromium:27: modified to accept an additional > parameter convertTextToURL:, which indicates > convertingTextToURL: My bad. Done.
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/2014733003/#ps180001 (title: "Review, round n + 2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2014733003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2014733003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, erikchen@chromium.org Link to the patchset: https://codereview.chromium.org/2014733003/#ps200001 (title: "Compilation fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2014733003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2014733003/200001
Message was sent while issue was closed.
Description was changed from ========== Removing parsing of text from pasteboard. This patch modifies logic for drag and dropping text that can be parsed as URL. Previously pasteboard would unconditionally answer that it contains URL. For some cases it is better to process such text using PasteAndGo logic. R=mark@chromium.org, pkasting@chromium.org BUG=610620 ========== to ========== Removing parsing of text from pasteboard. This patch modifies logic for drag and dropping text that can be parsed as URL. Previously pasteboard would unconditionally answer that it contains URL. For some cases it is better to process such text using PasteAndGo logic. R=mark@chromium.org, pkasting@chromium.org BUG=610620 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Removing parsing of text from pasteboard. This patch modifies logic for drag and dropping text that can be parsed as URL. Previously pasteboard would unconditionally answer that it contains URL. For some cases it is better to process such text using PasteAndGo logic. R=mark@chromium.org, pkasting@chromium.org BUG=610620 ========== to ========== Removing parsing of text from pasteboard. This patch modifies logic for drag and dropping text that can be parsed as URL. Previously pasteboard would unconditionally answer that it contains URL. For some cases it is better to process such text using PasteAndGo logic. R=mark@chromium.org, pkasting@chromium.org BUG=610620 Committed: https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957 Cr-Commit-Position: refs/heads/master@{#397689} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957 Cr-Commit-Position: refs/heads/master@{#397689} |
