Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2544933003:
Split methods for ClientHintsPreferences from FrameFetchContext (Closed)
|
Created:
4 years ago by tyoshino (SeeGerritForStatus) Modified:
4 years ago CC:
chromium-reviews, tyoshino+watch_chromium.org, blink-reviews-html_chromium.org, webcomponents-bugzilla_chromium.org, tfarina, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, gavinp+loader_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, Nate Chapin, loading-reviews_chromium.org, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSplit methods for ClientHintsPreferences from FrameFetchContext
FrameFetchContext is containing too many things. We'd like to slim it
down to hold basically only things that receive and handle notifications
from ResourceFetcher.
The methods for ClientHintsPreferences on FrameFetchContext is placed
there (I guess) just not to introduce dependencies from core/fetch to
core/loader, core/frame, etc. See
https://codereview.chromium.org/1189523015 which introduced this logic
initially.
----
Compatibility analysis:
It's safe to directly pass frame() to FrameClientHintsPreferencesContext
in FrameFetchContext::dispatchDidReceiveResponseInternal(). If the
passed ResourceFetcher is already clearContext()-ed, we never reach
dispatchDidReceiveResponseInternal(). So, we can assume that the
ResourceFetcher passed to updateFromAcceptClientHintsHeader() was
always not yet clearContext()-ed.
HttpEquiv::processHttpEquivAcceptCH() was passing document.fetcher()
to updateFromAcceptClientHintsHeader(). Document::m_fetcher is cleared
only in Document::shutdown(). Therefore, document.frame() being non-null
at the beginning of processHttpEquivAcceptCH() means that the Document
hasn't yet shutdown.
context().frame() call on document.fetcher() returns document.frame()
unless clearContext() was called. This is true even when
setImportsController() was called on the Document. This is clarified by
a separate CL https://codereview.chromium.org/2547843002/.
There're three callers of ResourceFetcher::clearContext(). One of them
is Document::shutdown(). So, we don't need to care it. The other cases
are:
- setImportsController() called by LinkImport::process() while
Document::loader() is null
- DocumentLoader::detachFromFrame() was called
However, I think this behavior was not intended initially and was
introduced just accidentally as stated above.
R=yhirano@chromium.org
BUG=671533
Committed: https://crrev.com/3b099320199a62fcb0e8b66f50e87fd1726c2449
Cr-Commit-Position: refs/heads/master@{#438090}
Patch Set 1 : a #
Total comments: 5
Patch Set 2 : Rebase #Patch Set 3 : Addressed #21 #Patch Set 4 : Use Member #Patch Set 5 : Rebase #Patch Set 6 : Rebase #Messages
Total messages: 46 (31 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
Description was changed from ========== Split methods for ClientHintsPreferences from FrameFetchContext BUG=none ========== to ========== Split methods for ClientHintsPreferences from FrameFetchContext FrameFetchContext is containing too many things. We'd like to slim it down to hold basically only things that receive and handle notifications from ResourceFetcher. It's safe to directly pass frame() to FrameClientHintsPreferencesContext in FrameFetchContext::dispatchDidReceiveResponseInternal(). If the passed ResourceFetcher is already clearContext()-ed, we never reach dispatchDidReceiveResponseInternal(). So, we can assume that the ResourceFetcher passed to updateFromAcceptClientHintsHeader() was always not yet clearContext()-ed. HttpEquiv::processHttpEquivAcceptCH() was passing document.fetcher() to updateFromAcceptClientHintsHeader(). Document::m_fetcher is cleared only in Document::shutdown(). Therefore, document.frame() being non-null at the beginning of processHttpEquivAcceptCH() means that the Document hasn't yet shutdown. context().frame() call on document.fetcher() returns document.frame() unless clearContext() was called. This is true even when setImportsController() was called on the Document. This is clarified by a separate CL https://codereview.chromium.org/2547843002/. There're three callers of ResourceFetcher::clearContext(). One of them is Document::shutdown(). So, we don't need to care it. The other cases are: - setImportsController() called by LinkImport::process() while Document::loader() is null - DocumentLoader::detachFromFrame() was called However, I this this behavior was not intended initially and was introduced just accidentally. This code was initially introduced in https://codereview.chromium.org/1189523015. ResourceFetcher and FetchContext was used just not to introduce dependency from core/fetch to core/loader. BUG=none ==========
Description was changed from ========== Split methods for ClientHintsPreferences from FrameFetchContext FrameFetchContext is containing too many things. We'd like to slim it down to hold basically only things that receive and handle notifications from ResourceFetcher. It's safe to directly pass frame() to FrameClientHintsPreferencesContext in FrameFetchContext::dispatchDidReceiveResponseInternal(). If the passed ResourceFetcher is already clearContext()-ed, we never reach dispatchDidReceiveResponseInternal(). So, we can assume that the ResourceFetcher passed to updateFromAcceptClientHintsHeader() was always not yet clearContext()-ed. HttpEquiv::processHttpEquivAcceptCH() was passing document.fetcher() to updateFromAcceptClientHintsHeader(). Document::m_fetcher is cleared only in Document::shutdown(). Therefore, document.frame() being non-null at the beginning of processHttpEquivAcceptCH() means that the Document hasn't yet shutdown. context().frame() call on document.fetcher() returns document.frame() unless clearContext() was called. This is true even when setImportsController() was called on the Document. This is clarified by a separate CL https://codereview.chromium.org/2547843002/. There're three callers of ResourceFetcher::clearContext(). One of them is Document::shutdown(). So, we don't need to care it. The other cases are: - setImportsController() called by LinkImport::process() while Document::loader() is null - DocumentLoader::detachFromFrame() was called However, I this this behavior was not intended initially and was introduced just accidentally. This code was initially introduced in https://codereview.chromium.org/1189523015. ResourceFetcher and FetchContext was used just not to introduce dependency from core/fetch to core/loader. BUG=none ========== to ========== Split methods for ClientHintsPreferences from FrameFetchContext FrameFetchContext is containing too many things. We'd like to slim it down to hold basically only things that receive and handle notifications from ResourceFetcher. The methods for ClientHintsPreferences on FrameFetchContext is placed there (I guess) just not to introduce dependencies from core/fetch to core/loader, core/frame, etc. See https://codereview.chromium.org/1189523015 which introduced this logic initially. ---- Compatibility analysis: It's safe to directly pass frame() to FrameClientHintsPreferencesContext in FrameFetchContext::dispatchDidReceiveResponseInternal(). If the passed ResourceFetcher is already clearContext()-ed, we never reach dispatchDidReceiveResponseInternal(). So, we can assume that the ResourceFetcher passed to updateFromAcceptClientHintsHeader() was always not yet clearContext()-ed. HttpEquiv::processHttpEquivAcceptCH() was passing document.fetcher() to updateFromAcceptClientHintsHeader(). Document::m_fetcher is cleared only in Document::shutdown(). Therefore, document.frame() being non-null at the beginning of processHttpEquivAcceptCH() means that the Document hasn't yet shutdown. context().frame() call on document.fetcher() returns document.frame() unless clearContext() was called. This is true even when setImportsController() was called on the Document. This is clarified by a separate CL https://codereview.chromium.org/2547843002/. There're three callers of ResourceFetcher::clearContext(). One of them is Document::shutdown(). So, we don't need to care it. The other cases are: - setImportsController() called by LinkImport::process() while Document::loader() is null - DocumentLoader::detachFromFrame() was called However, I this this behavior was not intended initially and was introduced just accidentally as stated above. BUG=none ==========
Patchset #1 (id:60001) has been deleted
The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run
Description was changed from ========== Split methods for ClientHintsPreferences from FrameFetchContext FrameFetchContext is containing too many things. We'd like to slim it down to hold basically only things that receive and handle notifications from ResourceFetcher. The methods for ClientHintsPreferences on FrameFetchContext is placed there (I guess) just not to introduce dependencies from core/fetch to core/loader, core/frame, etc. See https://codereview.chromium.org/1189523015 which introduced this logic initially. ---- Compatibility analysis: It's safe to directly pass frame() to FrameClientHintsPreferencesContext in FrameFetchContext::dispatchDidReceiveResponseInternal(). If the passed ResourceFetcher is already clearContext()-ed, we never reach dispatchDidReceiveResponseInternal(). So, we can assume that the ResourceFetcher passed to updateFromAcceptClientHintsHeader() was always not yet clearContext()-ed. HttpEquiv::processHttpEquivAcceptCH() was passing document.fetcher() to updateFromAcceptClientHintsHeader(). Document::m_fetcher is cleared only in Document::shutdown(). Therefore, document.frame() being non-null at the beginning of processHttpEquivAcceptCH() means that the Document hasn't yet shutdown. context().frame() call on document.fetcher() returns document.frame() unless clearContext() was called. This is true even when setImportsController() was called on the Document. This is clarified by a separate CL https://codereview.chromium.org/2547843002/. There're three callers of ResourceFetcher::clearContext(). One of them is Document::shutdown(). So, we don't need to care it. The other cases are: - setImportsController() called by LinkImport::process() while Document::loader() is null - DocumentLoader::detachFromFrame() was called However, I this this behavior was not intended initially and was introduced just accidentally as stated above. BUG=none ========== to ========== Split methods for ClientHintsPreferences from FrameFetchContext FrameFetchContext is containing too many things. We'd like to slim it down to hold basically only things that receive and handle notifications from ResourceFetcher. The methods for ClientHintsPreferences on FrameFetchContext is placed there (I guess) just not to introduce dependencies from core/fetch to core/loader, core/frame, etc. See https://codereview.chromium.org/1189523015 which introduced this logic initially. ---- Compatibility analysis: It's safe to directly pass frame() to FrameClientHintsPreferencesContext in FrameFetchContext::dispatchDidReceiveResponseInternal(). If the passed ResourceFetcher is already clearContext()-ed, we never reach dispatchDidReceiveResponseInternal(). So, we can assume that the ResourceFetcher passed to updateFromAcceptClientHintsHeader() was always not yet clearContext()-ed. HttpEquiv::processHttpEquivAcceptCH() was passing document.fetcher() to updateFromAcceptClientHintsHeader(). Document::m_fetcher is cleared only in Document::shutdown(). Therefore, document.frame() being non-null at the beginning of processHttpEquivAcceptCH() means that the Document hasn't yet shutdown. context().frame() call on document.fetcher() returns document.frame() unless clearContext() was called. This is true even when setImportsController() was called on the Document. This is clarified by a separate CL https://codereview.chromium.org/2547843002/. There're three callers of ResourceFetcher::clearContext(). One of them is Document::shutdown(). So, we don't need to care it. The other cases are: - setImportsController() called by LinkImport::process() while Document::loader() is null - DocumentLoader::detachFromFrame() was called However, I this this behavior was not intended initially and was introduced just accidentally as stated above. BUG=none ==========
tyoshino@chromium.org changed reviewers: + yhirano@chromium.org, yoav@yoav.ws
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Created a tracking bug for these efforts: http://crbug.com/671533
Description was changed from ========== Split methods for ClientHintsPreferences from FrameFetchContext FrameFetchContext is containing too many things. We'd like to slim it down to hold basically only things that receive and handle notifications from ResourceFetcher. The methods for ClientHintsPreferences on FrameFetchContext is placed there (I guess) just not to introduce dependencies from core/fetch to core/loader, core/frame, etc. See https://codereview.chromium.org/1189523015 which introduced this logic initially. ---- Compatibility analysis: It's safe to directly pass frame() to FrameClientHintsPreferencesContext in FrameFetchContext::dispatchDidReceiveResponseInternal(). If the passed ResourceFetcher is already clearContext()-ed, we never reach dispatchDidReceiveResponseInternal(). So, we can assume that the ResourceFetcher passed to updateFromAcceptClientHintsHeader() was always not yet clearContext()-ed. HttpEquiv::processHttpEquivAcceptCH() was passing document.fetcher() to updateFromAcceptClientHintsHeader(). Document::m_fetcher is cleared only in Document::shutdown(). Therefore, document.frame() being non-null at the beginning of processHttpEquivAcceptCH() means that the Document hasn't yet shutdown. context().frame() call on document.fetcher() returns document.frame() unless clearContext() was called. This is true even when setImportsController() was called on the Document. This is clarified by a separate CL https://codereview.chromium.org/2547843002/. There're three callers of ResourceFetcher::clearContext(). One of them is Document::shutdown(). So, we don't need to care it. The other cases are: - setImportsController() called by LinkImport::process() while Document::loader() is null - DocumentLoader::detachFromFrame() was called However, I this this behavior was not intended initially and was introduced just accidentally as stated above. BUG=none ========== to ========== Split methods for ClientHintsPreferences from FrameFetchContext FrameFetchContext is containing too many things. We'd like to slim it down to hold basically only things that receive and handle notifications from ResourceFetcher. The methods for ClientHintsPreferences on FrameFetchContext is placed there (I guess) just not to introduce dependencies from core/fetch to core/loader, core/frame, etc. See https://codereview.chromium.org/1189523015 which introduced this logic initially. ---- Compatibility analysis: It's safe to directly pass frame() to FrameClientHintsPreferencesContext in FrameFetchContext::dispatchDidReceiveResponseInternal(). If the passed ResourceFetcher is already clearContext()-ed, we never reach dispatchDidReceiveResponseInternal(). So, we can assume that the ResourceFetcher passed to updateFromAcceptClientHintsHeader() was always not yet clearContext()-ed. HttpEquiv::processHttpEquivAcceptCH() was passing document.fetcher() to updateFromAcceptClientHintsHeader(). Document::m_fetcher is cleared only in Document::shutdown(). Therefore, document.frame() being non-null at the beginning of processHttpEquivAcceptCH() means that the Document hasn't yet shutdown. context().frame() call on document.fetcher() returns document.frame() unless clearContext() was called. This is true even when setImportsController() was called on the Document. This is clarified by a separate CL https://codereview.chromium.org/2547843002/. There're three callers of ResourceFetcher::clearContext(). One of them is Document::shutdown(). So, we don't need to care it. The other cases are: - setImportsController() called by LinkImport::process() while Document::loader() is null - DocumentLoader::detachFromFrame() was called However, I this this behavior was not intended initially and was introduced just accidentally as stated above. BUG=671533 ==========
> However, I this this behavior was typo?
https://codereview.chromium.org/2544933003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameClientHintsPreferencesContext.h (right): https://codereview.chromium.org/2544933003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameClientHintsPreferencesContext.h:15: class FrameClientHintsPreferencesContext +final https://codereview.chromium.org/2544933003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameClientHintsPreferencesContext.h:27: Persistent<Frame> m_frame; I think a STACK_ALLOCATED class can have an onheap-member as Member without a trace function (but I couldn't find a document).
Description was changed from ========== Split methods for ClientHintsPreferences from FrameFetchContext FrameFetchContext is containing too many things. We'd like to slim it down to hold basically only things that receive and handle notifications from ResourceFetcher. The methods for ClientHintsPreferences on FrameFetchContext is placed there (I guess) just not to introduce dependencies from core/fetch to core/loader, core/frame, etc. See https://codereview.chromium.org/1189523015 which introduced this logic initially. ---- Compatibility analysis: It's safe to directly pass frame() to FrameClientHintsPreferencesContext in FrameFetchContext::dispatchDidReceiveResponseInternal(). If the passed ResourceFetcher is already clearContext()-ed, we never reach dispatchDidReceiveResponseInternal(). So, we can assume that the ResourceFetcher passed to updateFromAcceptClientHintsHeader() was always not yet clearContext()-ed. HttpEquiv::processHttpEquivAcceptCH() was passing document.fetcher() to updateFromAcceptClientHintsHeader(). Document::m_fetcher is cleared only in Document::shutdown(). Therefore, document.frame() being non-null at the beginning of processHttpEquivAcceptCH() means that the Document hasn't yet shutdown. context().frame() call on document.fetcher() returns document.frame() unless clearContext() was called. This is true even when setImportsController() was called on the Document. This is clarified by a separate CL https://codereview.chromium.org/2547843002/. There're three callers of ResourceFetcher::clearContext(). One of them is Document::shutdown(). So, we don't need to care it. The other cases are: - setImportsController() called by LinkImport::process() while Document::loader() is null - DocumentLoader::detachFromFrame() was called However, I this this behavior was not intended initially and was introduced just accidentally as stated above. BUG=671533 ========== to ========== Split methods for ClientHintsPreferences from FrameFetchContext FrameFetchContext is containing too many things. We'd like to slim it down to hold basically only things that receive and handle notifications from ResourceFetcher. The methods for ClientHintsPreferences on FrameFetchContext is placed there (I guess) just not to introduce dependencies from core/fetch to core/loader, core/frame, etc. See https://codereview.chromium.org/1189523015 which introduced this logic initially. ---- Compatibility analysis: It's safe to directly pass frame() to FrameClientHintsPreferencesContext in FrameFetchContext::dispatchDidReceiveResponseInternal(). If the passed ResourceFetcher is already clearContext()-ed, we never reach dispatchDidReceiveResponseInternal(). So, we can assume that the ResourceFetcher passed to updateFromAcceptClientHintsHeader() was always not yet clearContext()-ed. HttpEquiv::processHttpEquivAcceptCH() was passing document.fetcher() to updateFromAcceptClientHintsHeader(). Document::m_fetcher is cleared only in Document::shutdown(). Therefore, document.frame() being non-null at the beginning of processHttpEquivAcceptCH() means that the Document hasn't yet shutdown. context().frame() call on document.fetcher() returns document.frame() unless clearContext() was called. This is true even when setImportsController() was called on the Document. This is clarified by a separate CL https://codereview.chromium.org/2547843002/. There're three callers of ResourceFetcher::clearContext(). One of them is Document::shutdown(). So, we don't need to care it. The other cases are: - setImportsController() called by LinkImport::process() while Document::loader() is null - DocumentLoader::detachFromFrame() was called However, I think this behavior was not intended initially and was introduced just accidentally as stated above. BUG=671533 ==========
https://codereview.chromium.org/2544933003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameClientHintsPreferencesContext.h (right): https://codereview.chromium.org/2544933003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameClientHintsPreferencesContext.h:15: class FrameClientHintsPreferencesContext On 2016/12/08 07:47:12, yhirano wrote: > +final Done. https://codereview.chromium.org/2544933003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameClientHintsPreferencesContext.h:27: Persistent<Frame> m_frame; On 2016/12/08 07:47:12, yhirano wrote: > I think a STACK_ALLOCATED class can have an onheap-member as Member without a > trace function (but I couldn't find a document). Tried, but it seems we can't. ../../third_party/WebKit/Source/core/loader/FrameClientHintsPreferencesContext.h:28:3: note: [blink-gc] Raw pointer field 'm_frame' to a GC managed class declared here: Frame* m_frame;
On 2016/12/08 07:37:02, yhirano wrote: > > However, I this this behavior was > > typo? Fixed
Description was changed from ========== Split methods for ClientHintsPreferences from FrameFetchContext FrameFetchContext is containing too many things. We'd like to slim it down to hold basically only things that receive and handle notifications from ResourceFetcher. The methods for ClientHintsPreferences on FrameFetchContext is placed there (I guess) just not to introduce dependencies from core/fetch to core/loader, core/frame, etc. See https://codereview.chromium.org/1189523015 which introduced this logic initially. ---- Compatibility analysis: It's safe to directly pass frame() to FrameClientHintsPreferencesContext in FrameFetchContext::dispatchDidReceiveResponseInternal(). If the passed ResourceFetcher is already clearContext()-ed, we never reach dispatchDidReceiveResponseInternal(). So, we can assume that the ResourceFetcher passed to updateFromAcceptClientHintsHeader() was always not yet clearContext()-ed. HttpEquiv::processHttpEquivAcceptCH() was passing document.fetcher() to updateFromAcceptClientHintsHeader(). Document::m_fetcher is cleared only in Document::shutdown(). Therefore, document.frame() being non-null at the beginning of processHttpEquivAcceptCH() means that the Document hasn't yet shutdown. context().frame() call on document.fetcher() returns document.frame() unless clearContext() was called. This is true even when setImportsController() was called on the Document. This is clarified by a separate CL https://codereview.chromium.org/2547843002/. There're three callers of ResourceFetcher::clearContext(). One of them is Document::shutdown(). So, we don't need to care it. The other cases are: - setImportsController() called by LinkImport::process() while Document::loader() is null - DocumentLoader::detachFromFrame() was called However, I think this behavior was not intended initially and was introduced just accidentally as stated above. BUG=671533 ========== to ========== Split methods for ClientHintsPreferences from FrameFetchContext FrameFetchContext is containing too many things. We'd like to slim it down to hold basically only things that receive and handle notifications from ResourceFetcher. The methods for ClientHintsPreferences on FrameFetchContext is placed there (I guess) just not to introduce dependencies from core/fetch to core/loader, core/frame, etc. See https://codereview.chromium.org/1189523015 which introduced this logic initially. ---- Compatibility analysis: It's safe to directly pass frame() to FrameClientHintsPreferencesContext in FrameFetchContext::dispatchDidReceiveResponseInternal(). If the passed ResourceFetcher is already clearContext()-ed, we never reach dispatchDidReceiveResponseInternal(). So, we can assume that the ResourceFetcher passed to updateFromAcceptClientHintsHeader() was always not yet clearContext()-ed. HttpEquiv::processHttpEquivAcceptCH() was passing document.fetcher() to updateFromAcceptClientHintsHeader(). Document::m_fetcher is cleared only in Document::shutdown(). Therefore, document.frame() being non-null at the beginning of processHttpEquivAcceptCH() means that the Document hasn't yet shutdown. context().frame() call on document.fetcher() returns document.frame() unless clearContext() was called. This is true even when setImportsController() was called on the Document. This is clarified by a separate CL https://codereview.chromium.org/2547843002/. There're three callers of ResourceFetcher::clearContext(). One of them is Document::shutdown(). So, we don't need to care it. The other cases are: - setImportsController() called by LinkImport::process() while Document::loader() is null - DocumentLoader::detachFromFrame() was called However, I think this behavior was not intended initially and was introduced just accidentally as stated above. R=yhirano@chromium.org BUG=671533 ==========
https://codereview.chromium.org/2544933003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameClientHintsPreferencesContext.h (right): https://codereview.chromium.org/2544933003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameClientHintsPreferencesContext.h:27: Persistent<Frame> m_frame; On 2016/12/09 09:43:24, tyoshino wrote: > On 2016/12/08 07:47:12, yhirano wrote: > > I think a STACK_ALLOCATED class can have an onheap-member as Member without a > > trace function (but I couldn't find a document). > > Tried, but it seems we can't. > > ../../third_party/WebKit/Source/core/loader/FrameClientHintsPreferencesContext.h:28:3: > note: [blink-gc] Raw pointer field 'm_frame' to a GC managed class declared > here: > Frame* m_frame; Sorry I misunderstood the suggestion. Done
The CQ bit was checked by tyoshino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by tyoshino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2544933003/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tyoshino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2544933003/#ps180001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/13 05:19:12, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... LGTM
On 2016/12/13 06:29:11, Yoav Weiss wrote: > On 2016/12/13 05:19:12, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > LGTM Thank you Yoav.
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1481606326795540,
"parent_rev": "ef1a53a00b5b9336b0d3733a9c2a00aa185fbea4", "commit_rev":
"f53e41ce92cfeb551051f25790698f47ad2b2b0a"}
Message was sent while issue was closed.
Description was changed from ========== Split methods for ClientHintsPreferences from FrameFetchContext FrameFetchContext is containing too many things. We'd like to slim it down to hold basically only things that receive and handle notifications from ResourceFetcher. The methods for ClientHintsPreferences on FrameFetchContext is placed there (I guess) just not to introduce dependencies from core/fetch to core/loader, core/frame, etc. See https://codereview.chromium.org/1189523015 which introduced this logic initially. ---- Compatibility analysis: It's safe to directly pass frame() to FrameClientHintsPreferencesContext in FrameFetchContext::dispatchDidReceiveResponseInternal(). If the passed ResourceFetcher is already clearContext()-ed, we never reach dispatchDidReceiveResponseInternal(). So, we can assume that the ResourceFetcher passed to updateFromAcceptClientHintsHeader() was always not yet clearContext()-ed. HttpEquiv::processHttpEquivAcceptCH() was passing document.fetcher() to updateFromAcceptClientHintsHeader(). Document::m_fetcher is cleared only in Document::shutdown(). Therefore, document.frame() being non-null at the beginning of processHttpEquivAcceptCH() means that the Document hasn't yet shutdown. context().frame() call on document.fetcher() returns document.frame() unless clearContext() was called. This is true even when setImportsController() was called on the Document. This is clarified by a separate CL https://codereview.chromium.org/2547843002/. There're three callers of ResourceFetcher::clearContext(). One of them is Document::shutdown(). So, we don't need to care it. The other cases are: - setImportsController() called by LinkImport::process() while Document::loader() is null - DocumentLoader::detachFromFrame() was called However, I think this behavior was not intended initially and was introduced just accidentally as stated above. R=yhirano@chromium.org BUG=671533 ========== to ========== Split methods for ClientHintsPreferences from FrameFetchContext FrameFetchContext is containing too many things. We'd like to slim it down to hold basically only things that receive and handle notifications from ResourceFetcher. The methods for ClientHintsPreferences on FrameFetchContext is placed there (I guess) just not to introduce dependencies from core/fetch to core/loader, core/frame, etc. See https://codereview.chromium.org/1189523015 which introduced this logic initially. ---- Compatibility analysis: It's safe to directly pass frame() to FrameClientHintsPreferencesContext in FrameFetchContext::dispatchDidReceiveResponseInternal(). If the passed ResourceFetcher is already clearContext()-ed, we never reach dispatchDidReceiveResponseInternal(). So, we can assume that the ResourceFetcher passed to updateFromAcceptClientHintsHeader() was always not yet clearContext()-ed. HttpEquiv::processHttpEquivAcceptCH() was passing document.fetcher() to updateFromAcceptClientHintsHeader(). Document::m_fetcher is cleared only in Document::shutdown(). Therefore, document.frame() being non-null at the beginning of processHttpEquivAcceptCH() means that the Document hasn't yet shutdown. context().frame() call on document.fetcher() returns document.frame() unless clearContext() was called. This is true even when setImportsController() was called on the Document. This is clarified by a separate CL https://codereview.chromium.org/2547843002/. There're three callers of ResourceFetcher::clearContext(). One of them is Document::shutdown(). So, we don't need to care it. The other cases are: - setImportsController() called by LinkImport::process() while Document::loader() is null - DocumentLoader::detachFromFrame() was called However, I think this behavior was not intended initially and was introduced just accidentally as stated above. R=yhirano@chromium.org BUG=671533 Review-Url: https://codereview.chromium.org/2544933003 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Split methods for ClientHintsPreferences from FrameFetchContext FrameFetchContext is containing too many things. We'd like to slim it down to hold basically only things that receive and handle notifications from ResourceFetcher. The methods for ClientHintsPreferences on FrameFetchContext is placed there (I guess) just not to introduce dependencies from core/fetch to core/loader, core/frame, etc. See https://codereview.chromium.org/1189523015 which introduced this logic initially. ---- Compatibility analysis: It's safe to directly pass frame() to FrameClientHintsPreferencesContext in FrameFetchContext::dispatchDidReceiveResponseInternal(). If the passed ResourceFetcher is already clearContext()-ed, we never reach dispatchDidReceiveResponseInternal(). So, we can assume that the ResourceFetcher passed to updateFromAcceptClientHintsHeader() was always not yet clearContext()-ed. HttpEquiv::processHttpEquivAcceptCH() was passing document.fetcher() to updateFromAcceptClientHintsHeader(). Document::m_fetcher is cleared only in Document::shutdown(). Therefore, document.frame() being non-null at the beginning of processHttpEquivAcceptCH() means that the Document hasn't yet shutdown. context().frame() call on document.fetcher() returns document.frame() unless clearContext() was called. This is true even when setImportsController() was called on the Document. This is clarified by a separate CL https://codereview.chromium.org/2547843002/. There're three callers of ResourceFetcher::clearContext(). One of them is Document::shutdown(). So, we don't need to care it. The other cases are: - setImportsController() called by LinkImport::process() while Document::loader() is null - DocumentLoader::detachFromFrame() was called However, I think this behavior was not intended initially and was introduced just accidentally as stated above. R=yhirano@chromium.org BUG=671533 Review-Url: https://codereview.chromium.org/2544933003 ========== to ========== Split methods for ClientHintsPreferences from FrameFetchContext FrameFetchContext is containing too many things. We'd like to slim it down to hold basically only things that receive and handle notifications from ResourceFetcher. The methods for ClientHintsPreferences on FrameFetchContext is placed there (I guess) just not to introduce dependencies from core/fetch to core/loader, core/frame, etc. See https://codereview.chromium.org/1189523015 which introduced this logic initially. ---- Compatibility analysis: It's safe to directly pass frame() to FrameClientHintsPreferencesContext in FrameFetchContext::dispatchDidReceiveResponseInternal(). If the passed ResourceFetcher is already clearContext()-ed, we never reach dispatchDidReceiveResponseInternal(). So, we can assume that the ResourceFetcher passed to updateFromAcceptClientHintsHeader() was always not yet clearContext()-ed. HttpEquiv::processHttpEquivAcceptCH() was passing document.fetcher() to updateFromAcceptClientHintsHeader(). Document::m_fetcher is cleared only in Document::shutdown(). Therefore, document.frame() being non-null at the beginning of processHttpEquivAcceptCH() means that the Document hasn't yet shutdown. context().frame() call on document.fetcher() returns document.frame() unless clearContext() was called. This is true even when setImportsController() was called on the Document. This is clarified by a separate CL https://codereview.chromium.org/2547843002/. There're three callers of ResourceFetcher::clearContext(). One of them is Document::shutdown(). So, we don't need to care it. The other cases are: - setImportsController() called by LinkImport::process() while Document::loader() is null - DocumentLoader::detachFromFrame() was called However, I think this behavior was not intended initially and was introduced just accidentally as stated above. R=yhirano@chromium.org BUG=671533 Committed: https://crrev.com/3b099320199a62fcb0e8b66f50e87fd1726c2449 Cr-Commit-Position: refs/heads/master@{#438090} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3b099320199a62fcb0e8b66f50e87fd1726c2449 Cr-Commit-Position: refs/heads/master@{#438090} |
