713173 - Port |Bug 556563 - Disable/remove "View video" when you're already viewing the video| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Serge Gautherie (:sgautherie)]

I assume SeaMonkey wants this too.

(But let me complete bug 712871 first.)

Comment 1

[[e-mail]]

Works for me?

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #1)
> Works for me?

After a quick MXR search, I had the impression that application code had not been ported and I assumed it "should".
At least, test code has not been ported and I assume it should.

Both assumptions could prove to be wrong, but that needs more investigations/details.

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av0-WIP) Port bug 556563 ('inSyntheticDoc')

subtst_contextmenu.html: copied.
test_contextmenu.html: ported, but (2) frame part disabled ftb.
nsContextMenu.js: 3 ToDos...

http://mxr.mozilla.org/comm-central/search?string=onStandaloneImage&case=1&find=%2Fsuite%2F.*%2FnsContextMenu.js
SeaMonkey uses 'onStandaloneImage' a lot, whereas Firefox did/do not.
What should be done with these uses? (keep/replace/remove/...)

Comment 4

[[e-mail]]

Comment on attachment 584582 [details] [diff] [review]
(Av0-WIP) Port bug 556563 ('inSyntheticDoc')

>+/*
>+                          "frame",                null,
Odd, we should get a frame submenu for a context menu in a frame...

>+    video_in_iframe.pause();
[Why do we need this, can we not stop it from playing automatically?]

>+// ToDo: Should we keep 'this.mediaURL != this.target.ownerDocument.location.href'?
No, we don't need both checks.

>+// ToDo: Should be removed if all uses are removed or replaced with 'inSyntheticDoc'.
>     this.onStandaloneImage     = false;
Check with Philip Chee whether extensions are likely to use this.

>+    this.inSyntheticDoc =  this.target.ownerDocument.mozSyntheticDocument;
Nit: doubled space after =

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Port bug 556563 ('inSyntheticDoc'), test part [Checked in: Comment 7]

This patch updates the test (only), before porting/sync'ing related application code.


(In reply to [e-mail] from comment #1)
> Works for me?

It looks like the test does, yes.
(Nonetheless, I assume we should try to port/sync' application code too.)


(In reply to [e-mail] from comment #4)

> >+/*
> >+                          "frame",                null,
> Odd, we should get a frame submenu for a context menu in a frame...

Yes, I just had not bothered to do that part yet.

> >+    video_in_iframe.pause();
> [Why do we need this, can we not stop it from playing automatically?]

I assume this is not to depend on 'context-media-pause'/'context-media-play' items, when test runs fast/slow: I added a comment.
Firefox did it like that, I don't know how else to do it.

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av0a-WIP) Port bug 556563 ('inSyntheticDoc'), code part

This patch is to port/sync' the actual application code.


(In reply to Serge Gautherie (:sgautherie) from comment #3)
> nsContextMenu.js: 2 ToDos...
> 
> http://mxr.mozilla.org/comm-central/
> search?string=onStandaloneImage&case=1&find=%2Fsuite%2F.*%2FnsContextMenu.js
> SeaMonkey uses 'onStandaloneImage' a lot, whereas Firefox did/do not.
> What should be done with these uses? (keep/replace/remove/...)

Question still stands.

PS: On the other hand, I could try to sync' Firefox code if that's more how it should work.


(In reply to [e-mail] from comment #4)

> >+// ToDo: Should we keep 'this.mediaURL != this.target.ownerDocument.location.href'?
> No, we don't need both checks.

Removed.

> >+// ToDo: Should be removed if all uses are removed or replaced with 'inSyntheticDoc'.
> >     this.onStandaloneImage     = false;
> Check with Philip Chee whether extensions are likely to use this.

Firefox does not have this property anymore, so I assume we could remove it too, if we don't use it anymore in our code first.

> >+    this.inSyntheticDoc =  this.target.ownerDocument.mozSyntheticDocument;
> Nit: doubled space after =

Right: I saw it then forgot. Removed.

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 584713 [details] [diff] [review]
(Bv1) Port bug 556563 ('inSyntheticDoc'), test part
[Checked in: Comment 7]

http://hg.mozilla.org/comm-central/rev/e953d8a7e7fd

Comment 8

[Philip Chee]

>>>     this.onStandaloneImage     = false;
>> Check with Philip Chee whether extensions are likely to use this.
Most SeaMonkey extensions are Firefox extensions so if the extension is still being maintained the authors will be motivated to fix this.

However our context menu code appears to use this for image specific menu items.
1. Image specific menu items. Firefox assumes that .onLoadedImage subsumes .onStandaloneImage. Superficial testing seems to show that this is the case.

2. "context-fitimage" probably still needs onStandaloneImage.

Please file a bug to audit our remaining uses of onStandaloneImage. Thanks.

Comment 9

[Philip Chee]

Comment on attachment 584716 [details] [diff] [review]
(Av0a-WIP) Port bug 556563 ('inSyntheticDoc'), code part

f+ for this particular patch.

> +// ToDo: Should be removed if all uses are removed or replaced with 'inSyntheticDoc'.
I think we still need onStandaloneImage for "context-fitimage".

Comment 10

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Port bug 556563 ('inSyntheticDoc'), code (addition) part

This patch leaves other 'onStandaloneImage' uses alone (ftb).

Comment 11

[[e-mail]]

Comment on attachment 585929 [details] [diff] [review]
(Av1) Port bug 556563 ('inSyntheticDoc'), code (addition) part

>+    this.showItem("context-viewimage", (this.onImage &&
>+                   (!this.inSyntheticDoc || this.inFrame)) || this.onCanvas);
I know you added the extra ()s to clarify the precedence but now the wrapping looks weird. Could you put the this.onCanvas on the first line instead?

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Sync' some more (= fix 'context-sendimage') with FF bug 346849 [Checked in: Comment 16]

Sync' 2 codes<->comments. Make SM (behave) more like FF.

Ftr, in bug 346849
https://hg.mozilla.org/mozilla-central/rev/0caad1a2720a#l1.147
SM uses "aSkipPrompt = true" in both cases
and "aReferrer = null" for "this.onCanvas".
I assume both were intended as is, so I didn't sync' them.

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) Sync' 2 comments with FF bug 356590 [Checked in: Comment 17]

These come from
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/nsContextMenu.js&rev=1.1

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) Port bug 556563 ('inSyntheticDoc'), code (addition) part [Checked in: Comment 15]

Av1, with comment 11 suggestion(s).

(In reply to [e-mail] from comment #11)
> wrapping looks weird. Could you put the this.onCanvas on the first line
> instead?

Weird wrapping, I agree: just copied from FF.
I would rather use a 3rd line, to keep conditions in the current order.

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 585963 [details] [diff] [review]
(Av1a) Port bug 556563 ('inSyntheticDoc'), code (addition) part
[Checked in: Comment 15]

http://hg.mozilla.org/comm-central/rev/d85919a50594

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 585933 [details] [diff] [review]
(Cv1) Sync' some more (= fix 'context-sendimage') with FF bug 346849
[Checked in: Comment 16]

http://hg.mozilla.org/comm-central/rev/9728c7c02d7c

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 585936 [details] [diff] [review]
(Dv1) Sync' 2 comments with FF bug 356590
[Checked in: Comment 17]

http://hg.mozilla.org/comm-central/rev/e046e36e6796

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to Philip Chee from comment #8)
> Please file a bug to audit our remaining uses of onStandaloneImage. Thanks.

Bug 715681.