Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2322253004:
Drag and dropping text, parsable as url (Closed)
DescriptionView version of mac specific fix:
https://codereview.chromium.org/2014733003/
Some implementations of OSExchangeData::Provider
implicitly attempted to parse dropping text as url using
GURL parser. For some cases this is a correct behaviour,
for others autocomplete ought to be used to determine
destination URL. This requests adds a parametr to
the getters of OSExchangeData whether using GURL parser
is a desirable behaviour.
BUG=610620
Patch Set 1 : initial patch for windows #
Total comments: 2
Patch Set 2 : Moving url logic out of os_exchange_data #
Total comments: 13
Messages
Total messages: 44 (8 generated)
Description was changed from ========== View version of mac specific fix: https://codereview.chromium.org/2014733003/ Logic whether to attempt to parse plain url using GURL parser when doing drag and drop has been moved to the interface rather than being implicit. BUG=610620 ========== to ========== View version of mac specific fix: https://codereview.chromium.org/2014733003/ Logic whether to attempt to parse plain url using GURL parser when doing drag and drop has been moved to the interface rather than being implicit. BUG=610620 ==========
dyaroshev@yandex-team.ru changed reviewers: + erikchen@chromium.org, sadrul@chromium.org, sky@chromium.org
Created an patch. Unfortunatly some changes had to have been uploaded uncompiled - I wanted to get some initial feedback before monday and I didn't have enough time. Everything is done as close to the mac version as possible. Leftovers, if everything is ok: - modify other providers - I would like to move file_policy up the hierarchy, along with url_policy. This is a good idea because there is notable code duplication that has to modified on several platforms and it's inconsistent. - Some cleaning up.
https://codereview.chromium.org/2322253004/diff/1/chrome/test/base/interactiv... File chrome/test/base/interactive_test_utils.h (right): https://codereview.chromium.org/2322253004/diff/1/chrome/test/base/interactiv... chrome/test/base/interactive_test_utils.h:140: bool DragAnDrop(const gfx::Point& start, const gfx::Point& end); I forgot to revert changes in this file
sky@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for ui/base/dragdrop If you can get him happy I'll look at the rest
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
Does this represent a behavior change? If so what is the change, in detail? "Views version of Mac specific fix" is a rather poor initial summary line, and the rest of the CL description doesn't say. Why is the "implicit logic moved to the interface"? It's hard to evaluate whether that's correct when there's no information on what the purpose of that change is. I'm asking all this because I want to ensure we keep our API minimally complex and because there are a lot of subtleties to how we parse clipboard stuff for URL vs. not-URL and I want to ensure we don't break anything unintentionally.
Description was changed from ========== View version of mac specific fix: https://codereview.chromium.org/2014733003/ Logic whether to attempt to parse plain url using GURL parser when doing drag and drop has been moved to the interface rather than being implicit. BUG=610620 ========== to ========== View version of mac specific fix: https://codereview.chromium.org/2014733003/ Some implementations of OSExchangeData::Provider implicitly attempted to parse dropping text as url using GURL parser. For some cases this is a correct behaviour, for others autocomplete ought to be used to determine destination URL. This requests adds a parametr to the getters of OSExchangeData whether using GURL parser is a desirable behaviour. Logic whether to attempt to parse plain url using GURL parser when doing drag and drop has been moved to the interface rather than being implicit. BUG=610620 ==========
Description was changed from ========== View version of mac specific fix: https://codereview.chromium.org/2014733003/ Some implementations of OSExchangeData::Provider implicitly attempted to parse dropping text as url using GURL parser. For some cases this is a correct behaviour, for others autocomplete ought to be used to determine destination URL. This requests adds a parametr to the getters of OSExchangeData whether using GURL parser is a desirable behaviour. Logic whether to attempt to parse plain url using GURL parser when doing drag and drop has been moved to the interface rather than being implicit. BUG=610620 ========== to ========== View version of mac specific fix: https://codereview.chromium.org/2014733003/ Some implementations of OSExchangeData::Provider implicitly attempted to parse dropping text as url using GURL parser. For some cases this is a correct behaviour, for others autocomplete ought to be used to determine destination URL. This requests adds a parametr to the getters of OSExchangeData whether using GURL parser is a desirable behaviour. BUG=610620 ==========
Description was changed from ========== View version of mac specific fix: https://codereview.chromium.org/2014733003/ Some implementations of OSExchangeData::Provider implicitly attempted to parse dropping text as url using GURL parser. For some cases this is a correct behaviour, for others autocomplete ought to be used to determine destination URL. This requests adds a parametr to the getters of OSExchangeData whether using GURL parser is a desirable behaviour. BUG=610620 ========== to ========== View version of mac specific fix: https://codereview.chromium.org/2014733003/ Some implementations of OSExchangeData::Provider implicitly attempted to parse dropping text as url using GURL parser. For some cases this is a correct behaviour, for others autocomplete ought to be used to determine destination URL. This requests adds a parametr to the getters of OSExchangeData whether using GURL parser is a desirable behaviour. BUG=610620 ==========
On 2016/09/09 21:45:30, Peter Kasting wrote: > Does this represent a behavior change? If so what is the change, in detail? > "Views version of Mac specific fix" is a rather poor initial summary line, and > the rest of the CL description doesn't say. > There was a pretty big discussion in Mac CL. I assumed this would be just a follow up to that one. My bad. > Why is the "implicit logic moved to the interface"? It's hard to evaluate > whether that's correct when there's no information on what the purpose of that > change is. > > I'm asking all this because I want to ensure we keep our API minimally complex > and because there are a lot of subtleties to how we parse clipboard stuff for > URL vs. not-URL and I want to ensure we don't break anything unintentionally. I've updated CL description. I think it would be strange if mac/non mac similar api would differ in accepted parametrs.
On 2016/09/09 23:22:29, dyaroshev wrote: > On 2016/09/09 21:45:30, Peter Kasting wrote: > > Does this represent a behavior change? If so what is the change, in detail? > > "Views version of Mac specific fix" is a rather poor initial summary line, and > > the rest of the CL description doesn't say. > > > > There was a pretty big discussion in Mac CL. I assumed this would be just a > follow up to that one. > My bad. > > > Why is the "implicit logic moved to the interface"? It's hard to evaluate > > whether that's correct when there's no information on what the purpose of that > > change is. > > > > I'm asking all this because I want to ensure we keep our API minimally complex > > and because there are a lot of subtleties to how we parse clipboard stuff for > > URL vs. not-URL and I want to ensure we don't break anything unintentionally. > > I've updated CL description. I think it would be strange if mac/non mac similar > api > would differ in accepted parametrs. So wait, we always try to parse URLs, but sometimes we do it by running the text through the autocomplete system, which can do a variety of fixups, and sometimes we simply try to use GURL directly? How do we know we ever want to use GURL parsing? Are there cases where it's really correct to not do "nothing" and not do "run things through the autocomplete system to fix up and sanitize incoming input"?
On 2016/09/09 23:33:08, Peter Kasting wrote: > How do we know we ever want to use GURL parsing? Are there cases where it's > really correct to not do "nothing" and not do "run things through the > autocomplete system to fix up and sanitize incoming input"? Yes, there are. The original logic was that user can not add a query as a bookmark or as a home page, for example. And in cases where the only acceptable scenario is an URL, we would try really hard to parse inputed text as a URL.
On 2016/09/09 23:51:59, dyaroshev wrote: > On 2016/09/09 23:33:08, Peter Kasting wrote: > > How do we know we ever want to use GURL parsing? Are there cases where it's > > really correct to not do "nothing" and not do "run things through the > > autocomplete system to fix up and sanitize incoming input"? > > Yes, there are. The original logic was that user can not add a query as a > bookmark or as a home page, for example. And in cases where the only acceptable > scenario is an URL, we would try really hard to parse inputed text as a URL. A case like a bookmark or a home page should be run through fixup, or we won't get things right that need escaping, schemes added, etc. And that seems like the job of the caller. Why is the low-level drag and drop machinery trying to convert this?
On 2016/09/10 00:11:30, Peter Kasting wrote: > A case like a bookmark or a home page should be run through fixup, or we won't > get things right that need escaping, schemes added, etc. Can you refer me to an example of that? > And that seems like the job of the caller. Why is the low-level drag and drop > machinery trying to convert this? I would agree with you on that. Right now there is some weired GURL parsing logic going on inside that machinery. I don't like the solution we did on mac and that I'm trying to propogate here. But doing it properly requires quite serious man power and knowledge about specific components. It sertanly is too big for one cl. I suggest we intoduce a parametr that gives the caller the ability to disable weired parsing logic. Then we create refactoring tasks on people who know particular component. In this refactoring tasks we disable parsing in a specific place and add caller-side logic if necessary. After all places have been looked at, we clean out the parametr (parametrs). My point: there is a bad design decision (parsing text as URL) on which many places depend. Fixing all of them at once seems unrealistic. Let's hack and then plan a clean up.
On 2016/09/10 00:36:56, dyaroshev wrote: > On 2016/09/10 00:11:30, Peter Kasting wrote: > > A case like a bookmark or a home page should be run through fixup, or we won't > > get things right that need escaping, schemes added, etc. > > Can you refer me to an example of that? You mean, how one calls fixup? You're looking for url_formatter::FixupURL(), or, if you need to determine whether the input text is even a URL to begin with, AutocompleteClassifier::Classify(). > > And that seems like the job of the caller. Why is the low-level drag and drop > > machinery trying to convert this? > > I would agree with you on that. Right now there is some weired GURL parsing > logic going on inside that machinery. I don't like the solution we did on mac > and that I'm trying to propogate here. > > But doing it properly requires quite serious man power and knowledge about > specific components. It sertanly is too big for one cl. I suggest we intoduce a > parametr that gives the caller the ability to disable weired parsing logic. Then > we create refactoring tasks on people who know particular component. In this > refactoring tasks we disable parsing in a specific place and add caller-side > logic if necessary. After all places have been looked at, we clean out the > parametr (parametrs). > > My point: there is a bad design decision (parsing text as URL) on which many > places depend. Fixing all of them at once seems unrealistic. Let's hack and then > plan a clean up. Hack a fix then clean up is a fine plan if we have resources committed to the cleanup. If you don't have agreement from others to do it, and aren't doing it yourself, then there's a high likelihood it won't happen, in which case it's just the hack part. If you are agreeing to own the cleanup work and ensure it happens, then I'm fine with that idea.
On 2016/09/10 00:44:39, Peter Kasting wrote: > You mean, how one calls fixup? You're looking for url_formatter::FixupURL(), > or, if you need to determine whether the input text is even a URL to begin with, > AutocompleteClassifier::Classify(). Thanks. url_formatter is what I was looking for. > Hack a fix then clean up is a fine plan if we have resources committed to the > cleanup. If you don't have agreement from others to do it, and aren't doing it > yourself, then there's a high likelihood it won't happen, in which case it's > just the hack part. With this patch the code base would still be better: 1) It fixes the bug. 2) It improves on consistensy between platforms. 3) It's consistent with a similar hack with (file policy) in os_exchange_data. > If you are agreeing to own the cleanup work and ensure it happens, then I'm fine > with that idea. I don't think I have the resourses to do the clean up propperly, especially all the tests, any time soon. It's a lot of work and I don't have enough knowledge about all of the affected components. Here I looked at omnibox and browser_root_view -> disabled parsing. What are the alternatives to doing the hack? -Keeping the bug is certanly no good - bigger api is way better than one that has implicit very strange side effects. -Factoring the hackery better? If there is a way you like, let's do it, and I'll create a similar patch for mac later.
On 2016/09/10 07:12:37, dyaroshev wrote: > On 2016/09/10 00:44:39, Peter Kasting wrote: > > You mean, how one calls fixup? You're looking for url_formatter::FixupURL(), > > or, if you need to determine whether the input text is even a URL to begin > with, > > AutocompleteClassifier::Classify(). > > Thanks. url_formatter is what I was looking for. > > > Hack a fix then clean up is a fine plan if we have resources committed to the > > cleanup. If you don't have agreement from others to do it, and aren't doing > it > > yourself, then there's a high likelihood it won't happen, in which case it's > > just the hack part. > > With this patch the code base would still be better: > 1) It fixes the bug. Yes, that part is good. > 2) It improves on consistensy between platforms. That's not really a goal unless we plan to refactor to share code/APIs. It also assumes the Mac route is correct. I intentionally avoided looking at your functional changes on Mac. However, that's not an assumption I am necessarily willing to make. > 3) It's consistent with a similar hack with (file policy) in os_exchange_data. Again, I'm not sure consistency here is a goal, or that the existing code design is optimal. > > If you are agreeing to own the cleanup work and ensure it happens, then I'm > fine > > with that idea. > > I don't think I have the resourses to do the clean up propperly, especially all > the tests, > any time soon. It's a lot of work and I don't have enough knowledge about all of > the affected components. What do you mean by "all the tests"? This CL doesn't touch tests other than adding some new ones. So it's not clear how they're being affected and need clean up. It seems like the number of different objects passing PARSE_TEXT_AS_URL -- which are the ones about which we need to worry -- is relatively small and hopefully would not be that hard to manually test. The BrowserRootView already tries to fall back to a paste-and-go-based implementation when the drop data doesn't have a URL. OmniboxViewViews does something similar. It seems like that's what we want anywhere that wants to "do something navigational" with dropped data, like the tabstrip. If we want to refactor anything to be in the common OS exchange code, it seems like it would be this sort of thing. > Here I looked at omnibox and browser_root_view -> disabled parsing. > > What are the alternatives to doing the hack? > -Keeping the bug is certanly no good - bigger api is way better > than one that has implicit very strange side effects. Implicit effects are worrisome, but if we don't correctly fix the bug, or we fix one instance but not others/not similar problems, I'm not convinced we've bought much for our API complexification. And I don't know how this CL even fixes the bug in question, since the TabStrip is one of the places you're passing PARSE_TEXT_AS_URL, so trying to drag-and-drop onto a tab seems like it will do the wrong thing still. I don't think we should be passing a flag to GetURLAndTitle() to control this no matter what the implementation should look like. I would add a different method, like GetStringAsUrl(), that explicitly does the conversion here, so callers have to call it directly when they want this behavior. I would consider what HasURL() should do; callers like the home button probably want to reject "not-url-like" input, whereas things like the omnibox and browser root view are trying to accept everything and merely use that method to toggle between implementations. Perhaps if these implementations were pushed up to the OSExchangeData layer such calls could go away entirely and it would be more clear what HasURL() should do.
Invalid XSRF token. However, this was the data posted to the server: xsrf_token: 220e024e0f4ddef7e238d9ff6988cba5 add_as_reviewer: 0 message_only: 1 message: >What do you mean by "all the tests"? This CL doesn't touch tests other than > adding some new ones. So it's not clear how they're being affected and need > clean up. > By "all the tests" I mean that there are little to no tests on drag and drop. If we were to process most of the components individually, we would wright tests on them. But writing tests on drug and drop without is not an easy task - it took me about 2 days to write one for browser_root_view. > Implicit effects are worrisome, but if we don't correctly fix the bug, or we fix > one instance but not others/not similar problems, I'm not convinced we've bought > much for our API complexification. And I don't know how this CL even fixes the > bug in question, since the TabStrip is one of the places you're passing > PARSE_TEXT_AS_URL, so trying to drag-and-drop onto a tab seems like it will do > the wrong thing still. > Yes, it does seem like it. In this bug drag and drop on a tabstrip calls browser_root_view, which modifies os_exchange_data to contain an url. However I don't know if this is the only scenario when tab_strip::drag/drop functions get called. Because I'm not sure, I've decided to keep it functioning as is. It would be nice to have tests on this. > I don't think we should be passing a flag to GetURLAndTitle() to control this no > matter what the implementation should look like. I would add a different > method, like GetStringAsUrl(), that explicitly does the conversion here, so > callers have to call it directly when they want this behavior. I would consider > what HasURL() should do; callers like the home button probably want to reject > "not-url-like" input, whereas things like the omnibox and browser root view are > trying to accept everything and merely use that method to toggle between > implementations. Perhaps if these implementations were pushed up to the > OSExchangeData layer such calls could go away entirely and it would be more > clear what HasURL() should do. If we are doing a different from mac approach, it might be better to keep it not in os_exchange_data itself. I'll try out smth.
On 2016/09/13 14:15:04, dyaroshev wrote: > >What do you mean by "all the tests"? This CL doesn't touch tests other than > > adding some new ones. So it's not clear how they're being affected and need > > clean up. > > > By "all the tests" I mean that there are little to no tests on drag and drop. If > we were to process most of the components individually, we would wright tests on > them. > But writing tests on drug and drop without is not an easy task - it took me > about 2 days to write one for browser_root_view. While it would be nice to add tests for this stuff, I wouldn't block on this. > > Implicit effects are worrisome, but if we don't correctly fix the bug, or we fix > > one instance but not others/not similar problems, I'm not convinced we've bought > > much for our API complexification. And I don't know how this CL even fixes the > > bug in question, since the TabStrip is one of the places you're passing > > PARSE_TEXT_AS_URL, so trying to drag-and-drop onto a tab seems like it will do > > the wrong thing still. > > > Yes, it does seem like it. > In this bug drag and drop on a tabstrip calls browser_root_view, which modifies > os_exchange_data to contain an url. However I don't know if this is the only > scenario when tab_strip::drag/drop functions get called. Because I'm not sure, > I've decided to keep it functioning as is. It would be nice to have tests on > this. If you tested the cases of dropping onto a tab, dropping between tabs, and dropping onto an empty portion off the end of the strip, then you tested all the cases. If tabstrip DnD is handled by BrowserRotoView I don't know why Tabstrip would even have its own DnD code. > If we are doing a different from mac approach, it might be better to keep it not > in os_exchange_data itself. I'll try out smth. If this is The Right Way, we'd want Mac to use it too.
I'm not against moving this logic into OSExchangeData (after all, we already have a similar policy for filenames and whether or not they should be treated as URLs). However, it sounds like this canonicalization is more complicated than just testing if you can construct a valid GURL: maybe one way to do that is to allow a callback or function pointer to be passed in? https://codereview.chromium.org/2322253004/diff/1/ui/base/dragdrop/os_exchang... File ui/base/dragdrop/os_exchange_data.cc (right): https://codereview.chromium.org/2322253004/diff/1/ui/base/dragdrop/os_exchang... ui/base/dragdrop/os_exchange_data.cc:83: *url = std::move(test); GURL isn't actually movable, so this doesn't do anything (maybe it should be though?)
On 2016/09/13 19:53:26, dcheng wrote: > I'm not against moving this logic into OSExchangeData (after all, we already > have a similar policy for filenames and whether or not they should be treated as > URLs). > > However, it sounds like this canonicalization is more complicated than just > testing if you can construct a valid GURL: maybe one way to do that is to allow > a callback or function pointer to be passed in? I think that would make sense if we want different callers to try and "canonicalize" the text in different ways and, for some reason, they couldn't just do this themselves on return. But I don't think either of those conditions is true.
Once again - only win patch that requires some clean up. I have a few questions, if you could help me out - it would be great. Sorry about (right) comments I still don't understand your cl system completly https://codereview.chromium.org/2322253004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_strip.cc (right): https://codereview.chromium.org/2322253004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_strip.cc:1614: if (auto url_and_title = ui::TryToInterpretAsURL( I'm still not so sure, how much of this code should actually be deleted. What is with the CheckFileSupported logic here and drag and drop in browser_root_view? Who should be responsible for what? This logic with trying to parse an url here seems useless - we only get here from browser_root_view, that processes text already. Can we just get rid of it? https://codereview.chromium.org/2322253004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/home_button.cc (right): https://codereview.chromium.org/2322253004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/home_button.cc:163: return ui::CanBeInterpretedAsURL(data, ui::OSExchangeData::CONVERT_FILENAMES); Here we don't check for URL validity, but in OnPerformDrop we do. Do we ever want to get and invalid URL out of drag and drop? https://codereview.chromium.org/2322253004/diff/20001/chrome/test/base/intera... File chrome/test/base/interactive_test_utils.h (right): https://codereview.chromium.org/2322253004/diff/20001/chrome/test/base/intera... chrome/test/base/interactive_test_utils.h:140: bool DragAnDrop(const gfx::Point& start, const gfx::Point& end); Once again forgot to revert this file https://codereview.chromium.org/2322253004/diff/20001/components/bookmarks/br... File components/bookmarks/browser/bookmark_node_data_views.cc (right): https://codereview.chromium.org/2322253004/diff/20001/components/bookmarks/br... components/bookmarks/browser/bookmark_node_data_views.cc:59: if (auto url_and_title = ui::TryToInterpretAsURL( No checks on the validity of url here, unlike other places. Do we want them actually? https://codereview.chromium.org/2322253004/diff/20001/ui/base/DEPS File ui/base/DEPS (right): https://codereview.chromium.org/2322253004/diff/20001/ui/base/DEPS#newcode15 ui/base/DEPS:15: "+components/url_formatter/url_fixer.h", Where is the correct place to put drag_and_drop_url_utils? url_formatter and ui/base are independent right now https://codereview.chromium.org/2322253004/diff/20001/ui/base/dragdrop/os_exc... File ui/base/dragdrop/os_exchange_data_provider_win.cc (left): https://codereview.chromium.org/2322253004/diff/20001/ui/base/dragdrop/os_exc... ui/base/dragdrop/os_exchange_data_provider_win.cc:421: DCHECK(url->is_valid()); Am I correct that os_exchange_data GetURLAndTitle can not return invalid url? Why do most of the callers check? Should they not?
https://codereview.chromium.org/2322253004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_strip.cc (right): https://codereview.chromium.org/2322253004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_strip.cc:1614: if (auto url_and_title = ui::TryToInterpretAsURL( On 2016/09/14 17:12:05, dyaroshev wrote: > I'm still not so sure, how much of this code should actually be deleted. What is > with the CheckFileSupported logic here and drag and drop in browser_root_view? > Who should be responsible for what? > > This logic with trying to parse an url here seems useless - we only get here > from browser_root_view, that processes text already. Can we just get rid of it? Which logic are you referring to specifically? Maybe the clearest thing to do would be to write a small, separate CL in advance of this one that just rips out the bits here you think are redundant, so we can consider whether that refactoring change can Just Land. https://codereview.chromium.org/2322253004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/home_button.cc (right): https://codereview.chromium.org/2322253004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/home_button.cc:163: return ui::CanBeInterpretedAsURL(data, ui::OSExchangeData::CONVERT_FILENAMES); On 2016/09/14 17:12:05, dyaroshev wrote: > Here we don't check for URL validity, but in OnPerformDrop we do. Do we ever > want to get and invalid URL out of drag and drop? I would suggest that: * CanBeInterpretedAsURL() should only return true if the extracted URL would be valid. * TryToInterpretAsURL() should, similarly, include "and the URL is valid" as part of what it's returning. * Thus, callers of these should not need to check .is_valid(). The rationale for this is that a URL that is !is_valid() is not really a URL. So if the text cannot be interpreted as a valid URL, it cannot be interpreted as a URL at all. https://codereview.chromium.org/2322253004/diff/20001/components/bookmarks/br... File components/bookmarks/browser/bookmark_node_data_views.cc (right): https://codereview.chromium.org/2322253004/diff/20001/components/bookmarks/br... components/bookmarks/browser/bookmark_node_data_views.cc:59: if (auto url_and_title = ui::TryToInterpretAsURL( On 2016/09/14 17:12:05, dyaroshev wrote: > No checks on the validity of url here, unlike other places. Do we want them > actually? See other replies https://codereview.chromium.org/2322253004/diff/20001/ui/base/DEPS File ui/base/DEPS (right): https://codereview.chromium.org/2322253004/diff/20001/ui/base/DEPS#newcode15 ui/base/DEPS:15: "+components/url_formatter/url_fixer.h", On 2016/09/14 17:12:05, dyaroshev wrote: > Where is the correct place to put drag_and_drop_url_utils? url_formatter and > ui/base are independent right now I asked sky to comment here, he might know better. https://codereview.chromium.org/2322253004/diff/20001/ui/base/dragdrop/os_exc... File ui/base/dragdrop/os_exchange_data_provider_win.cc (left): https://codereview.chromium.org/2322253004/diff/20001/ui/base/dragdrop/os_exc... ui/base/dragdrop/os_exchange_data_provider_win.cc:421: DCHECK(url->is_valid()); On 2016/09/14 17:12:06, dyaroshev wrote: > Am I correct that os_exchange_data GetURLAndTitle can not return invalid url? You mean in the existing code? It looks to me like if GetUrl() fails, perhaps GetPlainTextURL() can succeed but with an invalid URL? In that case, this can return an invalid URL. Unless the code below lies -- if GetPlainTextURL() can't actually return true unless url->is_valid(), then you'd be right, we can't return an invalid URL. > Why do most of the callers check? Should they not? Similarly with my reply to one of your other questions, I think if we're asking for a URL and the function indicates whether it succeeded, that indication should only be "true" when the URL in question is valid. So I think callers should not have to check this, ideally.
https://codereview.chromium.org/2322253004/diff/20001/ui/base/DEPS File ui/base/DEPS (right): https://codereview.chromium.org/2322253004/diff/20001/ui/base/DEPS#newcode15 ui/base/DEPS:15: "+components/url_formatter/url_fixer.h", On 2016/09/14 17:12:05, dyaroshev wrote: > Where is the correct place to put drag_and_drop_url_utils? url_formatter and > ui/base are independent right now Components is a huge directory, and once you add this dependency two bad things could happen. url_formatter could get a dependency we don't want in ui/base and it won't readily be obvious to the person making the change. Second it confuses things from a mental model of what components is. By that I mean where does components sit in the layer tree of dependencies? From the developers perspective it's nice to think of components layering on top of ui/base, but that wouldn't be the case if you do that. For theses reasons ui/base shouldn't depend upon components. I see two options. Don't put ui/base/dragdrop/drag_and_drop_url_utils.h in ui/base, or move url_formatter into ui/base. Moving url_formatter into ui/base doesn't feel the greatest, but ui/base is already a bit of a dumping ground for functionality like this. https://codereview.chromium.org/2322253004/diff/20001/ui/base/dragdrop/drag_a... File ui/base/dragdrop/drag_and_drop_url_utils.cc (right): https://codereview.chromium.org/2322253004/diff/20001/ui/base/dragdrop/drag_a... ui/base/dragdrop/drag_and_drop_url_utils.cc:22: auto res = url_formatter::FixupURL(base::UTF16ToUTF8(text), ""); ""->std::string in all of these.
On 2016/09/15 21:26:53, Peter Kasting wrote: > https://codereview.chromium.org/2322253004/diff/20001/chrome/browser/ui/views... > File chrome/browser/ui/views/tabs/tab_strip.cc (right): > > https://codereview.chromium.org/2322253004/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/tabs/tab_strip.cc:1614: if (auto url_and_title = > ui::TryToInterpretAsURL( > On 2016/09/14 17:12:05, dyaroshev wrote: > > I'm still not so sure, how much of this code should actually be deleted. What > is > > with the CheckFileSupported logic here and drag and drop in browser_root_view? > > Who should be responsible for what? > > > > This logic with trying to parse an url here seems useless - we only get here > > from browser_root_view, that processes text already. Can we just get rid of > it? > > Which logic are you referring to specifically? > 1) Let's see if this is necessary. This parces text as url right now: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.c... I've left it unchanged, but it seems to be redundant. 2) You said that if browser_root_view deals with tab_strip drag and drop logic than, perphaps, tab_strip shouldn't have logic dealing with OsExchangeData. However this: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.c... is not duplicated in browser_root_view. Who should deal with drag and drop logic in tab_strip? Is it ok for both tab_stip and browser_root_view having some piece of functionality (don't refactor it). > > Here we don't check for URL validity, but in OnPerformDrop we do. Do we ever > > want to get and invalid URL out of drag and drop? > > I would suggest that: > > * CanBeInterpretedAsURL() should only return true if the extracted URL would be > valid. > * TryToInterpretAsURL() should, similarly, include "and the URL is valid" as > part of what it's returning. > * Thus, callers of these should not need to check .is_valid(). > This works for me. > > https://codereview.chromium.org/2322253004/diff/20001/ui/base/dragdrop/os_exc... > File ui/base/dragdrop/os_exchange_data_provider_win.cc (left): > > https://codereview.chromium.org/2322253004/diff/20001/ui/base/dragdrop/os_exc... > ui/base/dragdrop/os_exchange_data_provider_win.cc:421: DCHECK(url->is_valid()); > On 2016/09/14 17:12:06, dyaroshev wrote: > > Am I correct that os_exchange_data GetURLAndTitle can not return invalid url? > > You mean in the existing code? It looks to me like if GetUrl() fails, perhaps > GetPlainTextURL() can succeed but with an invalid URL? In that case, this can > return an invalid URL. Unless the code below lies -- if GetPlainTextURL() can't > actually return true unless url->is_valid(), then you'd be right, we can't > return an invalid URL. > > > Why do most of the callers check? Should they not? > > Similarly with my reply to one of your other questions, I think if we're asking > for a URL and the function indicates whether it succeeded, that indication > should only be "true" when the URL in question is valid. So I think callers > should not have to check this, ideally. GetPlainTextURL() doesn't return true if url is invalid. Ok - so get url will only return valid urls. My consern was - is it ok for such a low level component as OsExchangeData to deal with validity of urls. From your words I conclude that it is. Great.
On 2016/09/15 21:32:50, sky wrote: > https://codereview.chromium.org/2322253004/diff/20001/ui/base/DEPS > File ui/base/DEPS (right): > > https://codereview.chromium.org/2322253004/diff/20001/ui/base/DEPS#newcode15 > ui/base/DEPS:15: "+components/url_formatter/url_fixer.h", > On 2016/09/14 17:12:05, dyaroshev wrote: > > Where is the correct place to put drag_and_drop_url_utils? url_formatter and > > ui/base are independent right now > > Components is a huge directory, and once you add this dependency two bad things > could happen. url_formatter could get a dependency we don't want in ui/base and > it won't readily be obvious to the person making the change. Second it confuses > things from a mental model of what components is. By that I mean where does > components sit in the layer tree of dependencies? From the developers > perspective it's nice to think of components layering on top of ui/base, but > that wouldn't be the case if you do that. For theses reasons ui/base shouldn't > depend upon components. > > I see two options. Don't put ui/base/dragdrop/drag_and_drop_url_utils.h in > ui/base, or move url_formatter into ui/base. Moving url_formatter into ui/base > doesn't feel the greatest, but ui/base is already a bit of a dumping ground for > functionality like this. > Moving url_formatter in ui/base feels super wrong( I can use base::string16 as a paramentr to CanInterpret And TryToInterpret. Would that be better? The calling side would be slightly uglier, but not that much. > > https://codereview.chromium.org/2322253004/diff/20001/ui/base/dragdrop/drag_a... > ui/base/dragdrop/drag_and_drop_url_utils.cc:22: auto res = > url_formatter::FixupURL(base::UTF16ToUTF8(text), ""); > ""->std::string in all of these. I prefer {}? Is it ok?
If possible, try to reply by going back to where you left the question originally, expanding the comment you want to reply to, then using the "reply" link. For example, to reply to my reply in tab_strip.cc, visit https://codereview.chromium.org/2322253004/diff/20001/chrome/browser/ui/views... , click my comment to expand it, and click the "reply" link at the bottom, then edit/reply inline as needed.
On 2016/09/15 22:07:04, dyaroshev wrote: > On 2016/09/15 21:26:53, Peter Kasting wrote: > > > https://codereview.chromium.org/2322253004/diff/20001/chrome/browser/ui/views... > > File chrome/browser/ui/views/tabs/tab_strip.cc (right): > > > > > https://codereview.chromium.org/2322253004/diff/20001/chrome/browser/ui/views... > > chrome/browser/ui/views/tabs/tab_strip.cc:1614: if (auto url_and_title = > > ui::TryToInterpretAsURL( > > On 2016/09/14 17:12:05, dyaroshev wrote: > > > I'm still not so sure, how much of this code should actually be deleted. > What > > is > > > with the CheckFileSupported logic here and drag and drop in > browser_root_view? > > > Who should be responsible for what? > > > > > > This logic with trying to parse an url here seems useless - we only get here > > > from browser_root_view, that processes text already. Can we just get rid of > > it? > > > > Which logic are you referring to specifically? > > > > 1) Let's see if this is necessary. This parces text as url right now: > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.c... > I've left it unchanged, but it seems to be redundant. Right, I'm suggesting that if something about this is already redundant today, consider a separate CL that just attempts to remove the redundancy. It's then easy to e.g. run tryjobs or patch in that CL locally and test cases to see if it works. > 2) You said that if browser_root_view deals with tab_strip drag and drop logic > than, perphaps, tab_strip shouldn't have logic dealing with OsExchangeData. > However this: > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.c... > is not duplicated in browser_root_view. Who should deal with drag and drop logic > in tab_strip? Is it ok for both tab_stip and browser_root_view having some piece > of functionality (don't refactor it). I was responding to what sounded like a claim that the tabstrip code is being called by browser root view code that's already done the same things. In such cases, we shouldn't duplicate functionality. For the specific line you're pointing at, I've never looked at CheckFileSupported(), so I don't know whether it's necessary or what it does, or whether it can/should be refactored somewhere.
On 2016/09/15 22:14:48, dyaroshev wrote: > On 2016/09/15 21:32:50, sky wrote: > > https://codereview.chromium.org/2322253004/diff/20001/ui/base/DEPS > > File ui/base/DEPS (right): > > > > https://codereview.chromium.org/2322253004/diff/20001/ui/base/DEPS#newcode15 > > ui/base/DEPS:15: "+components/url_formatter/url_fixer.h", > > On 2016/09/14 17:12:05, dyaroshev wrote: > > > Where is the correct place to put drag_and_drop_url_utils? url_formatter and > > > ui/base are independent right now > > > > Components is a huge directory, and once you add this dependency two bad > things > > could happen. url_formatter could get a dependency we don't want in ui/base > and > > it won't readily be obvious to the person making the change. Second it > confuses > > things from a mental model of what components is. By that I mean where does > > components sit in the layer tree of dependencies? From the developers > > perspective it's nice to think of components layering on top of ui/base, but > > that wouldn't be the case if you do that. For theses reasons ui/base shouldn't > > depend upon components. > > > > I see two options. Don't put ui/base/dragdrop/drag_and_drop_url_utils.h in > > ui/base, or move url_formatter into ui/base. Moving url_formatter into ui/base > > doesn't feel the greatest, but ui/base is already a bit of a dumping ground > for > > functionality like this. > > > > Moving url_formatter in ui/base feels super wrong( I can use base::string16 as a > paramentr to CanInterpret And TryToInterpret. Would that be better? The calling > side would be slightly uglier, but not that much. Definitely don't move url_formatter into ui/base -- too many other places rely on that that can't depend on that. I would consider an alternative sky didn't suggest: Pull some of the drag-and-drop stuff out of ui/base into its own component. > > > https://codereview.chromium.org/2322253004/diff/20001/ui/base/dragdrop/drag_a... > > ui/base/dragdrop/drag_and_drop_url_utils.cc:22: auto res = > > url_formatter::FixupURL(base::UTF16ToUTF8(text), ""); > > ""->std::string in all of these. > > I prefer {}? Is it ok? Chromium convention is: Blah(std::string()); The closest thing in the team style guides is this guidance from https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-... : "Use C++11 "uniform init" syntax ("{}" without '=') only when neither [using = nor using ()] work." This is not exactly what you're asking about, since this is talking about initializing locals rather than constructing temps to pass to functions, but I would say, explicitly invoke the std::string() constructor here as is typical Chromium practice.
On 2016/09/15 22:48:52, Peter Kasting wrote: > On 2016/09/15 22:14:48, dyaroshev wrote: > I would consider an alternative sky didn't suggest: Pull some of the > drag-and-drop stuff out of ui/base into its own component. > components/drad_an_drop sounds alright? > Chromium convention is: > > Blah(std::string()); > Ok
On 2016/09/15 23:15:33, dyaroshev wrote: > On 2016/09/15 22:48:52, Peter Kasting wrote: > > On 2016/09/15 22:14:48, dyaroshev wrote: > > I would consider an alternative sky didn't suggest: Pull some of the > > drag-and-drop stuff out of ui/base into its own component. > > > > components/drad_an_drop sounds alright? components/drag_and_drop/ or components/dragdrop/ (to match existing directory name) would both be OK, I think.
On 2016/09/15 22:41:52, Peter Kasting wrote: > Right, I'm suggesting that if something about this is already redundant today, > consider a separate CL that just attempts to remove the redundancy. It's then > easy to e.g. run tryjobs or patch in that CL locally and test cases to see if it > works. I can't do that. The redundant logic is in attempt to parse text as a URL in a tabstrip model, it would never get text - browser_root_view will tranform it into OsExchangeData with an URL. In this CL redundant logic - parsing text as url - is exposed in tab_strip.cc, in current code it's deep in OsExchangeData. However, unlike browser_root_view or omnibox_edit_model there is no processing of text in tab_strip.cc itself, so it's not obvious if the author relied on text parsing logic in OsExchangeData. Seems like he or she hasn't. But I just wanted to clear this up. > I was responding to what sounded like a claim that the tabstrip code is being > called by browser root view code that's already done the same things. In such > cases, we shouldn't duplicate functionality. > > For the specific line you're pointing at, I've never looked at > CheckFileSupported(), so I don't know whether it's necessary or what it does, or > whether it can/should be refactored somewhere. Ok, I missunderstood you then. My bad.
On 2016/09/15 23:26:08, dyaroshev wrote: > On 2016/09/15 22:41:52, Peter Kasting wrote: > > Right, I'm suggesting that if something about this is already redundant today, > > consider a separate CL that just attempts to remove the redundancy. It's then > > easy to e.g. run tryjobs or patch in that CL locally and test cases to see if > it > > works. > > I can't do that. The redundant logic is in attempt to parse text as a URL in a > tabstrip model, it would never get text - browser_root_view will tranform it > into OsExchangeData with an URL. In this CL redundant logic - parsing text as > url - is exposed in tab_strip.cc, in current code it's deep in OsExchangeData. > However, unlike browser_root_view or omnibox_edit_model there is no processing > of text in tab_strip.cc itself, so it's not obvious if the author relied on text > parsing logic in OsExchangeData. Seems like he or she hasn't. But I just wanted > to clear this up. I'm still not really clear on what you're seeing as redundant. A specific reply like this would be useful: "Patch set 3 differs from patch set 2 only in that it eliminates what looks like redundant functionality." Then a reviewer can look at the delta between patches to see precisely what you're proposing changing.
On 2016/09/15 23:29:37, Peter Kasting wrote: > I'm still not really clear on what you're seeing as redundant. A specific reply > like this would be useful: > tab_strip.cc ui::TryToInterpretAsURL -> OsExchangeData::GetURLAndTitle. We don't have to interpret text as a URL here, there can be no text. I've just kept it in for the sake of refactoring being refactoring. > "Patch set 3 differs from patch set 2 only in that it eliminates what looks like > redundant functionality." Then a reviewer can look at the delta between patches > to see precisely what you're proposing changing. Ok, that works
On 2016/09/15 23:44:25, dyaroshev wrote: > On 2016/09/15 23:29:37, Peter Kasting wrote: > > I'm still not really clear on what you're seeing as redundant. A specific > reply > > like this would be useful: > > > tab_strip.cc ui::TryToInterpretAsURL -> OsExchangeData::GetURLAndTitle. We don't > have to interpret text as a URL here, there can be no text. I've just kept it in > for the sake of refactoring being refactoring. I think it's fine to try and make that change in this CL. I haven't looked at a call graph to verify your assertion that it is, in fact, OK :)
sadrul@chromium.org changed reviewers: - sadrul@chromium.org
dcheng@ and sky@'s review of ui/base/ would be sufficient here. Removing myself from the reviewer list.
What's the status of this CL? Who is it waiting on?
On 2017/02/11 01:59:38, Peter Kasting wrote: > What's the status of this CL? Who is it waiting on? Well - the bug is very minor and the fix requires a big refactoring. So this problem doesn't get scheduled.
On 2017/02/24 23:58:47, dyaroshev wrote: > On 2017/02/11 01:59:38, Peter Kasting wrote: > > What's the status of this CL? Who is it waiting on? > > Well - the bug is very minor and the fix requires a big refactoring. So this > problem doesn't get scheduled. Are you saying, you think the bug is valid and the patch should remain open and sitting on your plate, and you'll get to it later, or are you saying this CL should be closed because you're not going to come back to this?
On 2017/02/25 01:21:16, Peter Kasting wrote: > On 2017/02/24 23:58:47, dyaroshev wrote: > > On 2017/02/11 01:59:38, Peter Kasting wrote: > > > What's the status of this CL? Who is it waiting on? > > > > Well - the bug is very minor and the fix requires a big refactoring. So this > > problem doesn't get scheduled. > > Are you saying, you think the bug is valid and the patch should remain open and > sitting on your plate, and you'll get to it later, or are you saying this CL > should be closed because you're not going to come back to this? I'm willing to close the CL, if I am (or anybody else) will come to fix it in some future, they can create a new one. I'm curious - will it be recoverable? It has tests in it that seem to be good, if all possible I'd keep them stashed somewhere just in case.
On 2017/02/26 19:59:24, dyaroshev wrote: > On 2017/02/25 01:21:16, Peter Kasting wrote: > > On 2017/02/24 23:58:47, dyaroshev wrote: > > > On 2017/02/11 01:59:38, Peter Kasting wrote: > > > > What's the status of this CL? Who is it waiting on? > > > > > > Well - the bug is very minor and the fix requires a big refactoring. So this > > > problem doesn't get scheduled. > > > > Are you saying, you think the bug is valid and the patch should remain open > and > > sitting on your plate, and you'll get to it later, or are you saying this CL > > should be closed because you're not going to come back to this? > > I'm willing to close the CL, if I am (or anybody else) will come to fix it in > some future, they can create a new one. > I'm curious - will it be recoverable? It has tests in it that seem to be good, > if all possible I'd keep them stashed somewhere just in case. Closed CLs aren't deleted, they act just like landed CLs and remain visible in the review tool indefinitely. |
