Closed Bug 501154 Opened 11 years ago Closed 10 years ago

Improve clipboard event code

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: enndeakin, Assigned: enndeakin)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch work in progress (obsolete) — Splinter Review
This is some preliminary work to help with implementing bug 407983.

- consolidates the two places where cut/copy/paste events are fired from
- adds back most of the tests from bug 280959
- removes some needless indirection to the document viewer and presshell.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attached patch version 2 (obsolete) — Splinter Review
Bit more cleanup and fixes a memory access error that caused random crashes. Also now passes tests on the tryserver.
Attachment #385777 - Attachment is obsolete: true
Attachment #428276 - Flags: superreview?(neil)
Attachment #428276 - Flags: review?(Olli.Pettay)
Comment on attachment 428276 [details] [diff] [review]
consolidate code where clipboard events are fired

This isn't the right patch
Attachment #428276 - Flags: superreview?(neil)
Attachment #428276 - Flags: review?(Olli.Pettay)
Attached patch better patch (obsolete) — Splinter Review
Attachment #386272 - Attachment is obsolete: true
Attachment #428276 - Attachment is obsolete: true
Attachment #429157 - Flags: superreview?(neil)
Attachment #429157 - Flags: review?(Olli.Pettay)
Comment on attachment 429157 [details] [diff] [review]
better patch

I'm not keen on the name DoClipboardAction that only ever does the Copy part of the action, if applicable.

How are the cut and paste global window commands supposed to work?

Why is FirePasteEvent subtly different to DoCutOrCopy(NS_CUT)?
(In reply to comment #5)
> (From update of attachment 429157 [details] [diff] [review])
> I'm not keen on the name DoClipboardAction that only ever does the Copy part of
> the action, if applicable.
> 
> How are the cut and paste global window commands supposed to work?

They currently always return false for IsCommandEnabled, so effectively do nothing. Even if DoCommand is called directly (as key handlers do), it only fires the event, and has no other behaviour, which I think is a bug. Cut and paste only do anything in editors.
(In reply to comment #6)
> (In reply to comment #5)
> > How are the cut and paste global window commands supposed to work?
> They currently always return false for IsCommandEnabled
That's not true, they're used for designMode/contentEditable.
Attachment #429157 - Flags: superreview?(neil) → superreview-
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > How are the cut and paste global window commands supposed to work?
> > They currently always return false for IsCommandEnabled
> That's not true, they're used for designMode/contentEditable.

Editable areas use the editor controller (nsEditorCommands.cpp) which does respond with true as necessary.

The controller commands in GlobalWindowCommands.cpp only respond if an editor hasn't already done so, in which case, it is non-editable, and thus cut and paste aren't available.

The implementation of the latter is at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#2604 which just unconditionally returns false.

Note that I've tested that clipboard operations on designMode and contentEditable areas still work with this patch.
I'm not quite sure now whether I should review this, since the patch got
sr-. Neil (either one), should the patch be reviewed?
(In reply to comment #9)
> I'm not quite sure now whether I should review this, since the patch got
> sr-. Neil (either one), should the patch be reviewed?
Yes, although I'm still waiting for the answer to my last question.
I've combined FirePasteEvent and DoCutOrCopy into one method.
Attached patch version 5Splinter Review
Attachment #429157 - Attachment is obsolete: true
Attachment #431927 - Flags: superreview?(neil)
Attachment #431927 - Flags: review?(Olli.Pettay)
Attachment #429157 - Flags: review?(Olli.Pettay)
Attachment #431927 - Flags: superreview?(neil) → superreview+
Comment on attachment 431927 [details] [diff] [review]
version 5

>+
>+    /**
>+     * Retrieve the selection for the given presShell and document. If the
>+     * current focus within the document has its own selection, aSelection
>+     * will be set to it and this focused content node returned. Otherwise,
>+     * aSelection will be set to the document's selection and null will be
>+     * returned.
>+     */
>+    static nsIContent* GetSelectionForCopy(nsIPresShell* aPresShell,
>+                                           nsIDocument* aDocument,
>+                                           nsISelection** aSelection);
Since a document can have only one presshell, perhaps there isn't need
for the aPresShell parameter.

>+
>+  // It seems to be unsafe to fire an event handler during reflow (bug 393696)
>+  PRBool isReflowing = PR_TRUE;
>+  nsresult rv = presShell->IsReflowLocked(&isReflowing);
>+  if (NS_FAILED(rv) || isReflowing)
>+    return PR_FALSE;

Don't check IsReflowLocked, but nsContentUtils::IsSafeToRunScript()
And could you perhaps add warning if it isn't safe to run script.
Attachment #431927 - Flags: review?(Olli.Pettay) → review+
http://hg.mozilla.org/mozilla-central/rev/27259a0fcbe6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Depends on: 554230
I just ran across a crash that looks near where this code change happend.  regression?

http://crash-stats.mozilla.com/report/index/bp-dbda259d-64e6-4861-9e91-87d8c2100322

There was a lot going on when I hit the crash so I'm not sure how to reproduce.  I could have been loading this page http://www.networkworld.com/news/2010/031910-mozilla-confirms-critical-firefox.html
There is indeed a null check missing .
Depends on: 807599
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.