Issue 11364091: Fix ABP pages on refresh (Closed)
|
Created:
Aug. 8, 2013, 6:45 a.m. by Oleksandr Modified:
Aug. 13, 2013, 9:48 a.m. Visibility:
Public. |
DescriptionFix ABP pages on refresh
Patch Set 1 #
Total comments: 9
MessagesTotal messages: 13
This is a really simple fix. Just check if we should inject on both OnDownload and OnDocumentComplete. It does not fix any other problems becides refreshes on our FRP and Settings page (like ads on Google).
http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cpp File src/plugin/PluginTabBase.cpp (right): http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cp... src/plugin/PluginTabBase.cpp:149: DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_CREATE_SETTINGS_JAVASCRIPT, PLUGIN_ERROR_CREATE_SETTINGS_JAVASCRIPT_INVOKE, "CPluginTabBase::OnDocumentComplete - Failed to create Settings in JavaScript"); It's CPluginTabBase::InjectABP now, isn't it? http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cp... src/plugin/PluginTabBase.cpp:180: InjectABP(browser); So OnDocumentComplete is only called once, not on refresh? Is OnDownloadComplete called every time? Is it possible that DISP_DOCUMENTCOMPLETE is actually happening, but we don't call OnDocumentComplete because of the additional checks there?
http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cpp File src/plugin/PluginTabBase.cpp (right): http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cp... src/plugin/PluginTabBase.cpp:149: DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_CREATE_SETTINGS_JAVASCRIPT, PLUGIN_ERROR_CREATE_SETTINGS_JAVASCRIPT_INVOKE, "CPluginTabBase::OnDocumentComplete - Failed to create Settings in JavaScript"); It is, yes. On 2013/08/08 08:47:53, Felix H. Dahlke wrote: > It's CPluginTabBase::InjectABP now, isn't it? http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cp... src/plugin/PluginTabBase.cpp:180: InjectABP(browser); No, I have checked the events raised. It isn't fired. You can also check comments here: http://social.msdn.microsoft.com/Forums/ie/en-US/7a3a8d96-52f5-4ec0-a395-222c... On 2013/08/08 08:47:53, Felix H. Dahlke wrote: > So OnDocumentComplete is only called once, not on refresh? Is OnDownloadComplete > called every time? > > Is it possible that DISP_DOCUMENTCOMPLETE is actually happening, but we don't > call OnDocumentComplete because of the additional checks there?
http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cpp File src/plugin/PluginTabBase.cpp (right): http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cp... src/plugin/PluginTabBase.cpp:180: InjectABP(browser); On 2013/08/08 14:08:47, Oleksandr wrote: > No, I have checked the events raised. It isn't fired. You can also check > comments here: > http://social.msdn.microsoft.com/Forums/ie/en-US/7a3a8d96-52f5-4ec0-a395-222c... > > On 2013/08/08 08:47:53, Felix H. Dahlke wrote: > > So OnDocumentComplete is only called once, not on refresh? Is > OnDownloadComplete > > called every time? > > > > Is it possible that DISP_DOCUMENTCOMPLETE is actually happening, but we don't > > call OnDocumentComplete because of the additional checks there? > So, initially, OnDocumentComplete is fired and OnDownloadComplete is not fired, and on refresh OnDocumentComplete is not fired but OnDownloadComplete is fired? Doesn't make any sense to me, but if that's how it is...
On 2013/08/08 14:19:06, Felix H. Dahlke wrote: There are some troubles with this and your latest pipe changes. I get "Failed to write to pipe" with those applied.
On 2013/08/08 14:35:38, Oleksandr wrote: > On 2013/08/08 14:19:06, Felix H. Dahlke wrote: > > There are some troubles with this and your latest pipe changes. I get "Failed to > write to pipe" with those applied. Nevermind the last comment. It was a different engine version in my case.
http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cpp File src/plugin/PluginTabBase.cpp (right): http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cp... src/plugin/PluginTabBase.cpp:180: InjectABP(browser); No, I got it a bit wrong actually. Both events are fired in each case, but when refresh is done, the URL property is ambiguous (get_LocationURL returns about:blank on IE11, for example). So we can't count on it in that case. On the other hand, when not refreshing but just navigating - DOWNLOADCOMPLETE gets the URL that is in "incorrect" format. Like "c:\Program Files\..", not file:///c:/Program Files. We could just use one event and different URLs of course, but I think this is more consistent. On 2013/08/08 14:19:05, Felix H. Dahlke wrote: > On 2013/08/08 14:08:47, Oleksandr wrote: > > No, I have checked the events raised. It isn't fired. You can also check > > comments here: > > > http://social.msdn.microsoft.com/Forums/ie/en-US/7a3a8d96-52f5-4ec0-a395-222c... > > > > On 2013/08/08 08:47:53, Felix H. Dahlke wrote: > > > So OnDocumentComplete is only called once, not on refresh? Is > > OnDownloadComplete > > > called every time? > > > > > > Is it possible that DISP_DOCUMENTCOMPLETE is actually happening, but we > don't > > > call OnDocumentComplete because of the additional checks there? > > > > So, initially, OnDocumentComplete is fired and OnDownloadComplete is not fired, > and on refresh OnDocumentComplete is not fired but OnDownloadComplete is fired? > Doesn't make any sense to me, but if that's how it is...
http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cpp File src/plugin/PluginTabBase.cpp (right): http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cp... src/plugin/PluginTabBase.cpp:180: InjectABP(browser); On 2013/08/08 16:00:04, Oleksandr wrote: > No, I got it a bit wrong actually. Both events are fired in each case, but when > refresh is done, the URL property is ambiguous (get_LocationURL returns > about:blank on IE11, for example). So we can't count on it in that case. > On the other hand, when not refreshing but just navigating - DOWNLOADCOMPLETE > gets the URL that is in "incorrect" format. Like "c:\Program Files\..", not > file:///c:/Program Files. We could just use one event and different URLs of > course, but I think this is more consistent. > > On 2013/08/08 14:19:05, Felix H. Dahlke wrote: > > On 2013/08/08 14:08:47, Oleksandr wrote: > > > No, I have checked the events raised. It isn't fired. You can also check > > > comments here: > > > > > > http://social.msdn.microsoft.com/Forums/ie/en-US/7a3a8d96-52f5-4ec0-a395-222c... > > > > > > On 2013/08/08 08:47:53, Felix H. Dahlke wrote: > > > > So OnDocumentComplete is only called once, not on refresh? Is > > > OnDownloadComplete > > > > called every time? > > > > > > > > Is it possible that DISP_DOCUMENTCOMPLETE is actually happening, but we > > don't > > > > call OnDocumentComplete because of the additional checks there? > > > > > > > So, initially, OnDocumentComplete is fired and OnDownloadComplete is not > fired, > > and on refresh OnDocumentComplete is not fired but OnDownloadComplete is > fired? > > Doesn't make any sense to me, but if that's how it is... > That's fine by me, but isn't InjectABP being called twice per page load now? That we should avoid.
http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cpp File src/plugin/PluginTabBase.cpp (right): http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cp... src/plugin/PluginTabBase.cpp:180: InjectABP(browser); InjectABP can get called even more then twice per page load (it is fired on every time a document is downloaded, so all frames included). If the URL being navigated to fits the URL where ABP should be injected - the injection will run. It will always succeed (GetDispID and Invoke), but based on the current state of the page the actual Settings object will appear only in one of OnDownloadComplete or OnDocumentComplete events. It is not quite clear to me why the behavior is as such (Document's readyState is "complete" in both cases), but it works. Even if somehow injection ran twice, it wouldn't be a problem since it would just make sure the same object is created on page's javascript context. On 2013/08/08 16:20:59, Felix H. Dahlke wrote: > On 2013/08/08 16:00:04, Oleksandr wrote: > > No, I got it a bit wrong actually. Both events are fired in each case, but > when > > refresh is done, the URL property is ambiguous (get_LocationURL returns > > about:blank on IE11, for example). So we can't count on it in that case. > > On the other hand, when not refreshing but just navigating - DOWNLOADCOMPLETE > > gets the URL that is in "incorrect" format. Like "c:\Program Files\..", not > > file:///c:/Program Files. We could just use one event and different URLs of > > course, but I think this is more consistent. > > > > On 2013/08/08 14:19:05, Felix H. Dahlke wrote: > > > On 2013/08/08 14:08:47, Oleksandr wrote: > > > > No, I have checked the events raised. It isn't fired. You can also check > > > > comments here: > > > > > > > > > > http://social.msdn.microsoft.com/Forums/ie/en-US/7a3a8d96-52f5-4ec0-a395-222c... > > > > > > > > On 2013/08/08 08:47:53, Felix H. Dahlke wrote: > > > > > So OnDocumentComplete is only called once, not on refresh? Is > > > > OnDownloadComplete > > > > > called every time? > > > > > > > > > > Is it possible that DISP_DOCUMENTCOMPLETE is actually happening, but we > > > don't > > > > > call OnDocumentComplete because of the additional checks there? > > > > > > > > > > So, initially, OnDocumentComplete is fired and OnDownloadComplete is not > > fired, > > > and on refresh OnDocumentComplete is not fired but OnDownloadComplete is > > fired? > > > Doesn't make any sense to me, but if that's how it is... > > > > That's fine by me, but isn't InjectABP being called twice per page load now? > That we should avoid.
LGTM, with this nit fixed: http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cp...
LGTM but I would like the cleanup indicated below to be addressed at some point. http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cpp File src/plugin/PluginTabBase.cpp (right): http://codereview.adblockplus.org/11364091/diff/1/src/plugin/PluginTabBase.cp... src/plugin/PluginTabBase.cpp:124: if (pDoc2) Now that this is a separate function we can avoid the huge indentation here: if (!pDoc2) return; And similarly for all the other checks. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
