Closed Bug 713173 Opened 8 years ago Closed 8 years ago

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

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.9

People

(Reporter: sgautherie, Assigned: sgautherie)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

I assume SeaMonkey wants this too.

(But let me complete bug 712871 first.)
Works for me?
(In reply to neil@parkwaycc.co.uk 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.
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/...)
Attachment #584582 - Flags: feedback?(neil)
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 =
This patch updates the test (only), before porting/sync'ing related application code.


(In reply to neil@parkwaycc.co.uk 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 neil@parkwaycc.co.uk 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.
Attachment #584713 - Flags: review?(neil)
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 neil@parkwaycc.co.uk 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.
Attachment #584582 - Attachment is obsolete: true
Attachment #584582 - Flags: feedback?(neil)
Attachment #584716 - Flags: feedback?(philip.chee)
Attachment #584716 - Flags: feedback?(neil)
Attachment #584713 - Flags: review?(neil) → review+
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
Attachment #584713 - Attachment description: (Bv1) Port bug 556563 ('inSyntheticDoc'), test part. → (Bv1) Port bug 556563 ('inSyntheticDoc'), test part [Checked in: Comment 7]
>>>     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 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".
Attachment #584716 - Flags: feedback?(philip.chee) → feedback+
This patch leaves other 'onStandaloneImage' uses alone (ftb).
Attachment #584716 - Attachment is obsolete: true
Attachment #584716 - Flags: feedback?(neil)
Attachment #585929 - Flags: review?(neil)
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?
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.
Attachment #585933 - Flags: review?(neil)
Assignee: nobody → sgautherie.bz
No longer blocks: FF2SM
Status: NEW → ASSIGNED
Depends on: 346849
Target Milestone: --- → seamonkey2.9
Blocks: 715399
Av1, with comment 11 suggestion(s).

(In reply to neil@parkwaycc.co.uk 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.
Attachment #585929 - Attachment is obsolete: true
Attachment #585929 - Flags: review?(neil)
Attachment #585963 - Flags: review?(neil)
Attachment #585963 - Flags: review?(neil) → review+
Attachment #585933 - Flags: review?(neil) → review+
Attachment #585936 - Flags: review?(neil) → review+
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
Attachment #585963 - Attachment description: (Av1a) Port bug 556563 ('inSyntheticDoc'), code (addition) part → (Av1a) Port bug 556563 ('inSyntheticDoc'), code (addition) part [Checked in: Comment 15]
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
Attachment #585933 - Attachment description: (Cv1) Sync' some more (= fix 'context-sendimage') with FF bug 346849 → (Cv1) Sync' some more (= fix 'context-sendimage') with FF bug 346849 [Checked in: Comment 16]
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
Attachment #585936 - Attachment description: (Dv1) Sync' 2 comments with FF bug 356590 → (Dv1) Sync' 2 comments with FF bug 356590 [Checked in: Comment 17]
(In reply to Philip Chee from comment #8)
> Please file a bug to audit our remaining uses of onStandaloneImage. Thanks.

Bug 715681.
Blocks: 715681
No longer blocks: 715399
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
No longer depends on: 712871
Flags: in-testsuite+
Resolution: --- → FIXED
Blocks: 729633
You need to log in before you can comment on or make changes to this bug.