Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicate menu commands #1321

Closed
sizzlemctwizzle opened this issue Mar 27, 2011 · 9 comments
Closed

Duplicate menu commands #1321

sizzlemctwizzle opened this issue Mar 27, 2011 · 9 comments

Comments

@sizzlemctwizzle
Copy link
Contributor

See the thread for details and test case.

@sizzlemctwizzle
Copy link
Contributor Author

Patch.

@arantius
Copy link
Collaborator

It's not quite clear yet what's going on, but the adding/clearing of menu commands I've recently refactored is still not right.

I'd like to say simply and clearly that I'm not happy with changing to run scripts at pageshow rather than domcontentloaded. It's later, potentially much later for some pages. The right solution almost definitely uses a more reliable page-changed detection than pagehide. (I only started with that because that's what the original implementation seemed to use.)

@sizzlemctwizzle
Copy link
Contributor Author

I'm not happy with changing to run scripts at pageshow rather than domcontentloaded. It's later, potentially much later for some pages.
Could we just use the pageshow to add the menu commands, and go back to using DOMContentLoaded for running scripts?

@sizzlemctwizzle
Copy link
Contributor Author

I've updated my patch to run scripts on DOMContentLoaded but add menu commands on pageshow. I don't know if this is the solution you want but I think it solves this issue correctly.

@arantius
Copy link
Collaborator

arantius commented Apr 3, 2011

Test script: https://gist.github.com/898642 (you can ignore accelerator related stuff)

When I use this in FF 3.6 + GM 0.8, I see:

  1. Register a command, see it work
  2. Navigate to another page (in the same tab), see it still there, still work
  3. Hit back, still no change
    (broken)

or

  1. Register a command, see it work
  2. Open a new (blank) tab, see it disappear
  3. Switch back to the first tab
    (working)

or

  1. Navigate to any page
  2. Navigate to test page, register a command, see it work
  3. Press back, see it still there, still work
    (broken)

Or: bfcache completely confuses existing GM_rmc. The bfcache docs say that an unload handler will disable it, so I tried add a user script which registers an unload handler on every page.

The first test above sees the command disappear at step 2, but not come back at step 3. (Closer, but still broken.) Likewise the third behaves the same, disappearing on back but not reappearing on forward.

Or, ultimately: failing to take bfcache into accounte does not represent a regression. But it would be real nice to be able to. I have a patch in the works, started last week and accidentally destroyed by a sloppy git command, mostly back. I think it fully works w/ bfcache taken into account, in FF4. But I think it's broken-ish on FF3. Now that I'm more sure what the state before changes was in FF3+GM0.8, I'll compare to this, and then give it more thought.

@arantius
Copy link
Collaborator

arantius commented Apr 4, 2011

Here's what I've worked up for this issue:
arantius/greasemonkey@master...menu-commander

I still want to test a bit more in FF3, especially if it leaves the potential to leak memory in the menu commands list; FF3 does not give us the nice "window id" data to compare nor events to know about inner windows being destroyed, as far as I can tell.

@arantius
Copy link
Collaborator

arantius commented Apr 4, 2011

Updated my branch. Now, with this test script: https://gist.github.com/902547

I load tabs, I navigate to new pages, I close tabs, I hit forward and back; no matter how I repeat these actions, nor in what order, I'm seeing what I expect. And the pages that come from the bfcache are all working properly too. Tested in Firefox 3.6 and 4.0.

I see the "registered menus" array grow at very approximately about the size I expect the bfcache to. It never seems to grow to about 13 across the first 3 tabs, then each new tab (where all tabs have at least 5 pages in the history) only adds one more. Closing tabs makes this number drop.

@sizzlemctwizzle
Copy link
Contributor Author

I tested your branch with the test script in Firefox 3.6 and 4.0, and can confirm that everything worked as expected.

@arantius
Copy link
Collaborator

arantius commented Apr 5, 2011

Merge branch 'menu-commander'

Closed by 2b9ba9a

@arantius arantius closed this as completed Apr 5, 2011
poppen pushed a commit to poppen/greasemonkey that referenced this issue May 3, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants