patch to come with code for initial landing.

Comment on attachment 278704 [details] [diff] [review] v1.0 general: personally, I think you should drop the "moz" from the name. "storage-explorer" seems cleaner and we already have "moz" plastered all over stuff anyway. >+ >+<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#" >+ xmlns:em="http://www.mozilla.org/2004/em-rdf#"> >+ >+ <Description about="urn:mozilla:install-manifest"> >+ <em:id>mozStorage-explorer@mozilla.org</em:id> I don't know where this project "sits" relative to the mainline extensions like DOMi, but other SVN-based projects use "developer.mozilla.org" for their ID's. We could use some clarification here. I'd vote for "developer.mozilla.org" >+ >+<bindings id="storage-explorer-bindings" >+ xmlns="http://www.mozilla.org/xbl" >+ xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" >+ xmlns:xbl="http://www.mozilla.org/xbl"> >+ >+ <binding id="database-file" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem"> >+ <implementation> >+ <property name="file"> >+ <getter><![CDATA[ >+ return this.mFile; >+ ]]></getter> >+ <setter><![CDATA[ >+ this.mFile = val; >+ ]]></setter> >+ </property> >+ >+ <property name="connection"> >+ <getter><![CDATA[ >+ if (!this.mFile) >+ throw Components.results.NS_ERROR_UNEXPECTED; >+ if (this.mConnection) >+ return this.mConnection; >+ var ss = Components.classes["@mozilla.org/storage/service;1"]. >+ getService(Components.interfaces.mozIStorageService); >+ return mConnection = ss.openDatabase(this.mFile); >+ ]]></getter> >+ </property> >+ </implementation> >+ >+ <content> >+ <xul:label xbl:inherits="value=name"/> >+ </content> >+ </binding> >+ >+</bindings> I was almost ready to say that this XBL seemed overkill, but the "connection" property seems to be doing something useful. The rest could be handled as JS properties on the DOM element. >+ >+ <menupopup id="menu_ToolsPopup"> >+ <menuitem id="menu-storage-explorer" label="&Explorer.title;" >+ accesskey="&Explorer.accesskey;" insertafter="devToolsSeparator" >+ oncommand="toOpenWindowByType('StorageExplorer:Main', >+ 'chrome://storage-explorer/content/storage-explorer.xul', >+ 'chrome,dialog=no,resizeable');"/> Something strange here. When I open the window, it isn't resizable, although from this code I would expect it to be resizable. >+ /** >+ * Performs a query on the selected database. >+ */ >+ performQuery: function performQuery() >+ { >+ this.clearNotifications(); >+ var text = document.getElementById("query-text"); >+ if (!text.value) { >+ this.dispatchNotification(this.notificationStrings.getString("noQueryEntered")); >+ return; >+ } >+ >+ var dbConn = document.getElementById("databases").selectedItem; >+ if (!dbConn) { >+ this.dispatchNotification(this.notificationStrings.getString("noDatabaseSelected")); >+ return; >+ } >+ dbConn = dbConn.connection; >+ >+ // First we have to remove any existing data the tree has. You should also initially hide the errorbox, in case it was visible from a previous query >+ */ >+ dispatchNotification: function dispatchNotification(aMessage) >+ { >+ var box = document.getElementById("notifications"); >+ box.appendNotification(aMessage, "", "", box.PRIORITY_INFO_LOW, []); >+ }, You may want to check to see if a duplicate notification is already displayed. They seem to pile up, but only 1 is ever shown. Or clear notifications before adding a new one or at the start of "performQuery" >+ >+ /** >+ * Clears all notifications from the notification box. >+ */ >+ clearNotifications: function clearNotifications() >+ { >+ var box = document.getElementById("notifications"); >+ box.removeAllNotifications(false); >+ }, r- until we decide the name and id, since that impacts a lot of files. Otherwise, the code looks good. Who is the module owner (or group) for this project anyway? To help us make these decisions.

(In reply to comment #2) > (From update of attachment 278704 [details] [diff] [review]) > general: personally, I think you should drop the "moz" from the name. > "storage-explorer" seems cleaner and we already have "moz" plastered all over > stuff anyway. yeah, internally that's what I've already been doing :/ > I don't know where this project "sits" relative to the mainline extensions like > DOMi, but other SVN-based projects use "developer.mozilla.org" for their ID's. > We could use some clarification here. I'd vote for "developer.mozilla.org" I'm fairly indifferent here, but since this this a developer tool, I suppose the latter makes more sense. > I was almost ready to say that this XBL seemed overkill, but the "connection" > property seems to be doing something useful. The rest could be handled as JS > properties on the DOM element. It didn't seem to be working for me setting the file with JS properties, which is why I added the binding. I also plan to do a bit more with the binding like add a tooltip with the full path. > Something strange here. When I open the window, it isn't resizable, although > from this code I would expect it to be resizable. I'll look into this. > >+ performQuery: function performQuery() > >+ { > >+ this.clearNotifications(); > You should also initially hide the errorbox, in case it was visible from a > previous query I am pretty sure that this.clearNotifications() does that, no? > You may want to check to see if a duplicate notification is already displayed. > They seem to pile up, but only 1 is ever shown. > > Or clear notifications before adding a new one or at the start of > "performQuery" I think it may be best to let callers handle that because they may want to stack them. > Who is the module owner (or group) for this project anyway? To help us make > these decisions. Not sure of "Other Applications" has a module owner to be honest. Maybe gavin knows?

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 278704 [details] [diff] [review] [details]) > > general: personally, I think you should drop the "moz" from the name. > > "storage-explorer" seems cleaner and we already have "moz" plastered all over > > stuff anyway. > yeah, internally that's what I've already been doing :/ > > > I don't know where this project "sits" relative to the mainline extensions like > > DOMi, but other SVN-based projects use "developer.mozilla.org" for their ID's. > > We could use some clarification here. I'd vote for "developer.mozilla.org" > I'm fairly indifferent here, but since this this a developer tool, I suppose > the latter makes more sense. > > > I was almost ready to say that this XBL seemed overkill, but the "connection" > > property seems to be doing something useful. The rest could be handled as JS > > properties on the DOM element. > It didn't seem to be working for me setting the file with JS properties, which > is why I added the binding. I also plan to do a bit more with the binding like > add a tooltip with the full path. > > > Something strange here. When I open the window, it isn't resizable, although > > from this code I would expect it to be resizable. > I'll look into this. > > > >+ performQuery: function performQuery() > > >+ { > > >+ this.clearNotifications(); > > You should also initially hide the errorbox, in case it was visible from a > > previous query > I am pretty sure that this.clearNotifications() does that, no? yeah, I was testing an older version. I should have looked closer at the code > > > You may want to check to see if a duplicate notification is already displayed. > > They seem to pile up, but only 1 is ever shown. > > > > Or clear notifications before adding a new one or at the start of > > "performQuery" > I think it may be best to let callers handle that because they may want to > stack them. this may have been an artifact of me using an older version too > > > Who is the module owner (or group) for this project anyway? To help us make > > these decisions. > Not sure of "Other Applications" has a module owner to be honest. Maybe gavin > knows? > I wasn't thinking of the module owner of 'Other Apps' as much as who drives the direction of the various tools in 'Other App'. Since the tools are so varied, I was assuming that it may not be 1 person for all the apps. - Once you make a decision on the name and ID, this code is ready to checkin.

note: I should be using %d instead of %S for my number of results string.

Alexander, can you comment on the a11y of this code perhaps?

I can't get the resizer to display at all, so I'd like to leave that in a follow-up bug.

(In reply to comment #5) > note: I should be using %d instead of %S for my number of results string. and no, this didn't work at all :(

helps if I get all the names fixed...

Comment on attachment 279281 [details] [diff] [review] v1.2 >+ <binding id="database-file" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem"> >+ <implementation> >+ <property name="file"> >+ <getter><![CDATA[ >+ return this.mFile; >+ ]]></getter> >+ <setter><![CDATA[ >+ this.mFile = val; >+ ]]></setter> >+ </property> nit: I think setters should return the value. (at least I have seen Enn changing code to do it) >+<!DOCTYPE overlay SYSTEM "chrome://storage-explorer/locale/storage-explorer.dtd"> >+<overlay id="storage-explorer-overlay" >+ xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> >+ >+ <menupopup id="menu_ToolsPopup"> >+ <menuitem id="menu-storage-explorer" label="&Explorer.title;" >+ accesskey="&Explorer.accesskey;" insertafter="devToolsSeparator" >+ oncommand="toOpenWindowByType('StorageExplorer:Main', >+ 'chrome://storage-explorer/content/storage-explorer.xul', >+ 'chrome,dialog=no,resizeable');"/> >+ </menupopup> >+ >+</overlay> it's "resizable", not "resizeable" :), then the window resizes nicely >+ <notificationbox id="notifications"/> >+ >+ <hbox flex="1"> >+ <vbox> >+ <label value="&Databases.label;"/> >+ <richlistbox id="databases"/> >+ </vbox> I still think the <richlistbox> should flex so it fills the space vertically >+ } else { >+ // No data was returned >+ let errbox = document.getElementById("query-results-message"); >+ errbox.value = this.resultStrings.getString("query.noResults"); >+ errbox.hidden = false; >+ tree.hidden = true; >+ numResults.hidden = true; >+ return; >+ } >+ } >+ }, is the 'return' needed? >+ // We traverse the entire profile directory for sqlite databases >+ var ProfD = Cc["@mozilla.org/file/directory_service;1"]. >+ getService(Ci.nsIProperties).get("ProfD", Ci.nsIFile); >+ var result = []; >+ for (let f in dirIter(ProfD)) { >+ if (f.isDirectory()) { >+ for (let f2 in dirIter(f)) >+ if (isDatabase(f2)) >+ result.push(f2); >+ } else if (isDatabase(f)) >+ result.push(f); >+ } you're setting the coding style here, but we normally use curly braces in a single line 'if', 'else if' or 'else' when any of the other blocks need curly braces. This code looks ready to land. Make the changes you want. r=mfinkle

(In reply to comment #10) > is the 'return' needed? yes, otherwise we'll execute the try block in the next function. > you're setting the coding style here, but we normally use curly braces in a > single line 'if', 'else if' or 'else' when any of the other blocks need curly > braces. I agree with you actually, but I've been told differently in the past. I'm gonna brace it though because I find it clearer.

for checkin

Adding Makefile.in Adding content Adding content/overlay.xul Adding content/storage-explorer.css Adding content/storage-explorer.js Adding content/storage-explorer.xul Adding content/widgets.xml Adding install.rdf Adding jar.mn Adding locale Adding locale/Makefile.in Adding locale/en-US Adding locale/en-US/notifications.properties Adding locale/en-US/results.properties Adding locale/en-US/storage-explorer.dtd Adding locale/jar.mn Transmitting file data ............. Committed revision 6505.