Closed Bug 501154 Opened 11 years ago Closed 10 years ago
Improve clipboard event code
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.
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
Comment on attachment 428276 [details] [diff] [review] consolidate code where clipboard events are fired This isn't the right patch
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.
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+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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 .
You need to log in before you can comment on or make changes to this bug.