Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1530153002:
Change how DOM Inspector fetches document's base URL. (Closed)
|
Created:
5 years ago by mharanczyk Modified:
5 years ago CC:
apavlov+blink_chromium.org, blink-reviews, blink-reviews-dom_chromium.org, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, dglazkov+blink, eae+blinkwatch, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, rwlbuis, sergeyv+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange how DOM Inspector fetches document's base URL.
With integration of https://codereview.chromium.org/1409293007 appending
empty string is invalid operation for non hierarchical base urls. Since
DOM Inspector Agent actually want to determine base URL ask for that
data directly.
Without this change base url for non hierarchical (data:) urls was
always empty for inspector, which in turn caused webdriver to freeze
when trying to communicate with such documents, because it wrongly
assumed it is still in loading state (base url was empty),
so it waited for load complete.
Committed: https://crrev.com/30bd0e0d48f3775e3214ad3e524356259f58f580
Cr-Commit-Position: refs/heads/master@{#366079}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Added documentation. #
Messages
Total messages: 21 (8 generated)
Description was changed from ========== Change how DOM Inspector fetches document's base URL. With integration of https://codereview.chromium.org/1409293007 appending empty string is invalid operation for non hierarchical base urls. Since DOM Inspector Agent actually want to determine base URL ask for that data directly. Without this change base url for non hierarchical (data:) urls was always empty for inspector, which in turn caused webdriver to freeze when trying to communicate with such documents, because it wrongly assumed it is still in loading state (base url was empty), so it waited for load complete. ========== to ========== Change how DOM Inspector fetches document's base URL. With integration of https://codereview.chromium.org/1409293007 appending empty string is invalid operation for non hierarchical base urls. Since DOM Inspector Agent actually want to determine base URL ask for that data directly. Without this change base url for non hierarchical (data:) urls was always empty for inspector, which in turn caused webdriver to freeze when trying to communicate with such documents, because it wrongly assumed it is still in loading state (base url was empty), so it waited for load complete. ==========
mharanczyk@opera.com changed reviewers: + dgozman@chromium.org, pfeldman@chromium.org
PTAL, this change addresses webdriver freeze on data: documents.
dgozman@chromium.org changed reviewers: + tkent@chromium.org
+tkent for core/dom https://codereview.chromium.org/1530153002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/1530153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1478: return document->baseURLForOverride(document->baseURL()).string(); This change looks fine, but I'm not sure about baseURLForOverride naming and changes in Document in general. Over to tkent@.
https://codereview.chromium.org/1530153002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/1530153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1478: return document->baseURLForOverride(document->baseURL()).string(); On 2015/12/16 18:00:49, dgozman wrote: > This change looks fine, but I'm not sure about baseURLForOverride naming and > changes in Document in general. Over to tkent@. I'm open for suggestions about naming, I'm not happy with it either. I tried to preserve old code paths without duplicating code/logic, but maybe it is enough to just return document->baseURL().string()? Doing empty string completeURL magic can lead to some unfortunate failures when all Inspector wants in baseURL.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/1530153002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/1530153002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1478: return document->baseURLForOverride(document->baseURL()).string(); On 2015/12/16 18:29:42, mharanczyk wrote: > On 2015/12/16 18:00:49, dgozman wrote: > > This change looks fine, but I'm not sure about baseURLForOverride naming and > > changes in Document in general. Over to tkent@. > > I'm open for suggestions about naming, I'm not happy with it either. > I tried to preserve old code paths without duplicating code/logic, but maybe it > is enough to just return document->baseURL().string()? Doing empty string > completeURL magic can lead to some unfortunate failures when all Inspector wants > in baseURL. Will this not return the parent document's base url if document->baseURL() is about:blank? that is not what's wanted here.
On 2015/12/16 18:53:27, sof wrote: > https://codereview.chromium.org/1530153002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): > > https://codereview.chromium.org/1530153002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1478: return > document->baseURLForOverride(document->baseURL()).string(); > On 2015/12/16 18:29:42, mharanczyk wrote: > > On 2015/12/16 18:00:49, dgozman wrote: > > > This change looks fine, but I'm not sure about baseURLForOverride naming and > > > changes in Document in general. Over to tkent@. > > > > I'm open for suggestions about naming, I'm not happy with it either. > > I tried to preserve old code paths without duplicating code/logic, but maybe > it > > is enough to just return document->baseURL().string()? Doing empty string > > completeURL magic can lead to some unfortunate failures when all Inspector > wants > > in baseURL. > > Will this not return the parent document's base url if document->baseURL() is > about:blank? that is not what's wanted here. It was always working like this with old code, intended or not, hence my question if just using document->baseURL().string() is not better. I left this logic untouched, just split the function into two to remove the append empty string part that started failing as it seems to me it is unneeded in this case anyway. Some time ago I've stumbled on a case where about:blank url got assigned to new frames temporarily on creation before actual url was loaded, maybe this is to prevent it from returning wrong data?
On 2015/12/16 19:02:40, mharanczyk wrote: > On 2015/12/16 18:53:27, sof wrote: > > > https://codereview.chromium.org/1530153002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): > > > > > https://codereview.chromium.org/1530153002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1478: return > > document->baseURLForOverride(document->baseURL()).string(); > > On 2015/12/16 18:29:42, mharanczyk wrote: > > > On 2015/12/16 18:00:49, dgozman wrote: > > > > This change looks fine, but I'm not sure about baseURLForOverride naming > and > > > > changes in Document in general. Over to tkent@. > > > > > > I'm open for suggestions about naming, I'm not happy with it either. > > > I tried to preserve old code paths without duplicating code/logic, but maybe > > it > > > is enough to just return document->baseURL().string()? Doing empty string > > > completeURL magic can lead to some unfortunate failures when all Inspector > > wants > > > in baseURL. > > > > Will this not return the parent document's base url if document->baseURL() is > > about:blank? that is not what's wanted here. > > It was always working like this with old code, intended or not, hence my > question if just using document->baseURL().string() is not better. I left this > logic untouched, just split the function into two to remove the append empty > string part that started failing as it seems to me it is unneeded in this case > anyway. Some time ago I've stumbled on a case where about:blank url got assigned > to new frames temporarily on creation before actual url was loaded, maybe this > is to prevent it from returning wrong data? Right, the change here is in-line with that. It'd be good to have the about:blank treatment clarified; it seems unusual to not propagate the base URL as-is, but don't take my word for it.
lgtm > KURL completeURLWithOverride(const String&, const KURL& baseURLOverride) const; > const KURL& baseURLForOverride(const KURL& baseURLOverride) const; I want documentation comments on these functions.
tkent@, added documentation, I'm not sure if this is what you wanted so please take a look again and advise. dgozman@, I think I still need your official approval for core/inspector part, also could you answer my question if returning parent document base url in some circumstances is intentional and desired here or maybe it is enough it use document->baseURL()?
On 2015/12/17 12:34:24, mharanczyk wrote: > tkent@, added documentation, I'm not sure if this is what you wanted so please > take a look again and advise. > > dgozman@, I think I still need your official approval for core/inspector part, > also could you answer my question if returning parent document base url in some > circumstances is intentional and desired here or maybe it is enough it use > document->baseURL()? core/inspector lgtm I think for regular users baseURL() would suffice. But automation like webdriver may probably be confused if we return about:blank while document is being loaded. Let's preserve the behavior.
The comments looks good. Thanks.
The CQ bit was checked by mharanczyk@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org Link to the patchset: https://codereview.chromium.org/1530153002/#ps20001 (title: "Added documentation.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1530153002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1530153002/20001
Message was sent while issue was closed.
Description was changed from ========== Change how DOM Inspector fetches document's base URL. With integration of https://codereview.chromium.org/1409293007 appending empty string is invalid operation for non hierarchical base urls. Since DOM Inspector Agent actually want to determine base URL ask for that data directly. Without this change base url for non hierarchical (data:) urls was always empty for inspector, which in turn caused webdriver to freeze when trying to communicate with such documents, because it wrongly assumed it is still in loading state (base url was empty), so it waited for load complete. ========== to ========== Change how DOM Inspector fetches document's base URL. With integration of https://codereview.chromium.org/1409293007 appending empty string is invalid operation for non hierarchical base urls. Since DOM Inspector Agent actually want to determine base URL ask for that data directly. Without this change base url for non hierarchical (data:) urls was always empty for inspector, which in turn caused webdriver to freeze when trying to communicate with such documents, because it wrongly assumed it is still in loading state (base url was empty), so it waited for load complete. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Change how DOM Inspector fetches document's base URL. With integration of https://codereview.chromium.org/1409293007 appending empty string is invalid operation for non hierarchical base urls. Since DOM Inspector Agent actually want to determine base URL ask for that data directly. Without this change base url for non hierarchical (data:) urls was always empty for inspector, which in turn caused webdriver to freeze when trying to communicate with such documents, because it wrongly assumed it is still in loading state (base url was empty), so it waited for load complete. ========== to ========== Change how DOM Inspector fetches document's base URL. With integration of https://codereview.chromium.org/1409293007 appending empty string is invalid operation for non hierarchical base urls. Since DOM Inspector Agent actually want to determine base URL ask for that data directly. Without this change base url for non hierarchical (data:) urls was always empty for inspector, which in turn caused webdriver to freeze when trying to communicate with such documents, because it wrongly assumed it is still in loading state (base url was empty), so it waited for load complete. Committed: https://crrev.com/30bd0e0d48f3775e3214ad3e524356259f58f580 Cr-Commit-Position: refs/heads/master@{#366079} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/30bd0e0d48f3775e3214ad3e524356259f58f580 Cr-Commit-Position: refs/heads/master@{#366079} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
