Issue 6325312296058880: issue #290 - [seamonkey] blockable items list persists on tab changes (Closed)
|
Created:
July 6, 2014, 12:42 p.m. by saroyanm Modified:
July 8, 2014, 12:21 p.m. Reviewers:
Wladimir Palant Visibility:
Public. |
DescriptionThis issue is related to current ticket:
https://issues.adblockplus.org/ticket/290
Patch Set 1 #
Total comments: 3
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #MessagesTotal messages: 11
Wladimir can you please have a look when you will have time. http://codereview.adblockplus.org/6325312296058880/diff/5629499534213120/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/6325312296058880/diff/5629499534213120/lib/... lib/appSupport.js:376: onLocationChange: function(progress, request, uri) The issue looks caused by onLocationChange method's aFlags attribute. While in Firefox it's implemented as optional attribute: http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsIWebProgressListener.idl#344 In Seamonkey the implementation for the attribute is missing: http://mxr.mozilla.org/seamonkey/source/uriloader/base/nsIWebProgressListener.idl#322 Not sure if this is the best approach, also not sure if there is a better way to determine whether the listener is triggered by anchor scroll, while it's being hard to determine if the page was reloaded (ctrl+r) or moved to anchor.
http://codereview.adblockplus.org/6325312296058880/diff/5629499534213120/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/6325312296058880/diff/5629499534213120/lib/... lib/appSupport.js:376: onLocationChange: function(progress, request, uri) The code you are linking to is very outdated, the description on MXR says: "This the old code in CVS for Gecko, XULRunner, Firefox, Thunderbird, Calendar, Camino, and SeaMonkey. CVS trunk is only used for Gecko 1.9.0 / Firefox 3 and the 1.9.0.* / 3.0.* security releases." For current SeaMonkey and Thunderbird releases you should be looking at the comm-central repository: http://mxr.mozilla.org/comm-central/source/mozilla/uriloader/base/nsIWebProgr... Note than an additional parameter wouldn't be an issue, our code is backwards-compatible and can deal with that parameter not being there. The issue is really that the flag is there in SeaMonkey and its value is 1 (LOCATION_CHANGE_SAME_DOCUMENT). This appears to be a SeaMonkey bug, we'll need to check it.
http://codereview.adblockplus.org/6325312296058880/diff/5629499534213120/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/6325312296058880/diff/5629499534213120/lib/... lib/appSupport.js:376: onLocationChange: function(progress, request, uri) On 2014/07/07 12:21:21, Wladimir Palant wrote: > releases you should be looking at the comm-central repository: > http://mxr.mozilla.org/comm-central/source/mozilla/uriloader/base/nsIWebProgr... Thanks Wladimir for pointing on correct repository, my bad went on big Seamonkey text :) will take into consideration for future. Anyway I have a doubt, why in that case aFlag always 1 for seamonkey should we also consider it as a bug in Seamonkey ? Anyway will have a closer look on that also. > Note than an additional parameter wouldn't be an issue, our code is > backwards-compatible and can deal with that parameter not being there. The issue > is really that the flag is there in SeaMonkey and its value is 1 > (LOCATION_CHANGE_SAME_DOCUMENT). This appears to be a SeaMonkey bug, we'll need > to check it. Good point, BTW LOCATION_CHANGE_SAME_DOCUMENT flag's value is 1 also for Firefox not only Seamonkey.
I confirmed this as a SeaMonkey bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1035171. The corresponding Firefox code calls this method with 0 as flags. We'll need to come up with some work-around for SeaMonkey. One option would be to ignore the flags in SeaMonkey but that will clear the blockable items list every time the user navigates to an anchor (meaning LOCATION_CHANGE_SAME_DOCUMENT flag is used correctly).
On 2014/07/07 13:46:56, Wladimir Palant wrote: > I confirmed this as a SeaMonkey bug: > https://bugzilla.mozilla.org/show_bug.cgi?id=1035171. The corresponding Firefox > code calls this method with 0 as flags. Cool. Wladimir can you please let me know why I do not have access to the bug you have mentioned ? Is it a private bug ? https://bugzilla.mozilla.org/show_bug.cgi?id=782516 Getting current message - You are not authorized to access bug #782516 > We'll need to come up with some work-around for SeaMonkey. One option would be > to ignore the flags in SeaMonkey but that will clear the blockable items list > every time the user navigates to an anchor (meaning > LOCATION_CHANGE_SAME_DOCUMENT flag is used correctly). Actually If we will pass ignoreSameDoc variable as false with addBrowserLocationListener method for Seamonkey in that case the list will be reloaded for the anchor redirect I guess, not clear. Maybe we can go with it ?
> Actually If we will pass ignoreSameDoc variable as false with addBrowserLocationListener method for Seamonkey in that case the list will be reloaded for the anchor redirect I guess, not clear. > Maybe we can go with it ? Sorry, was little not correct: So for now I guess with current patch we are ignoring the flags, so maybe we can go with current patch until the issue will be fixed ? Anyway for now the list is being reloaded on anchor scroll.
On 2014/07/07 14:05:10, saroyanm wrote: > Cool. Wladimir can you please let me know why I do not have access to the bug > you have mentioned ? Is it a private bug ? > https://bugzilla.mozilla.org/show_bug.cgi?id=782516 As I mentioned, I don't have access to it either - it's a security issue. I merely saw that it is responsible in the change history. > Actually If we will pass ignoreSameDoc variable as false with > addBrowserLocationListener method for Seamonkey in that case the list will be > reloaded for the anchor redirect I guess, not clear. > Maybe we can go with it ? I guess we'll have to ignore the same-doc flag in SeaMonkey, yes. But we should do that without duplicating the entire listener - maybe indeed by defining an ignoreSameDoc flag which would be initially set to false but overridden with true in the SeaMonkey branch. We should also link to the bug that makes this hack necessary (and hopefully add a condition soon to remove it in newer SeaMonkey versions).
Thanks for your comments Wladimir, can you please check the new patch. Hope I've got your idea right.
http://codereview.adblockplus.org/6325312296058880/diff/5657382461898752/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/6325312296058880/diff/5657382461898752/lib/... lib/appSupport.js:370: // for Seamonkey we have to ignore same document flag because of bug #1035171 Please use a link, otherwise it won't be obvious where to look up this bug number (our bug database, Mozilla's, Chrome's). http://codereview.adblockplus.org/6325312296058880/diff/5657382461898752/lib/... lib/appSupport.js:372: }; Forcing the ignoreSameDoc parameter to be always false seems to be a sensible approach. However, I would prefer keeping the entire hack in the SeaMonkey section then. Something along these lines: let origAddBrowserLocationListener = exports.addBrowserLocationListener; exports.addBrowserLocationListener = function sm_addBrowserLocationListener(window, callback, ignoreSameDoc) { origAddBrowserLocationListener(window, callback, false); };
New patch uploaded, hope we can give a go this time. http://codereview.adblockplus.org/6325312296058880/diff/5657382461898752/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/6325312296058880/diff/5657382461898752/lib/... lib/appSupport.js:370: // for Seamonkey we have to ignore same document flag because of bug #1035171 On 2014/07/08 11:29:02, Wladimir Palant wrote: > Please use a link, otherwise it won't be obvious where to look up this bug > number (our bug database, Mozilla's, Chrome's). Done. http://codereview.adblockplus.org/6325312296058880/diff/5657382461898752/lib/... lib/appSupport.js:372: }; On 2014/07/08 11:29:02, Wladimir Palant wrote: > Forcing the ignoreSameDoc parameter to be always false seems to be a sensible > approach. However, I would prefer keeping the entire hack in the SeaMonkey > section then. Something along these lines: > > let origAddBrowserLocationListener = exports.addBrowserLocationListener; > exports.addBrowserLocationListener = function > sm_addBrowserLocationListener(window, callback, ignoreSameDoc) > { > origAddBrowserLocationListener(window, callback, false); > }; I like this :)
LGTM 
|
