5:28 AM <aswan> um. this page is showing a bunch of code changes as diffs in the commit message... https://reviewboard.mozilla.org/r/117362/diff/1-2/ 5:30 AM <aswan> i think the fragment labeled as a diff in commit-message is actually a diff in toolkit/components/Extension.jsm

Commit message diff is a fake FileDiff. These are created during the push and should be displayed as a first FileDiff in the list. If MozReview knows that a commit message filediff is present it is using JS and CSS to rename its unique filename to "commit-message". In this case no commit message FileDiff is returned by the server, but front-end has received an information that one will be displayed and renamed the "toolkit/components/Extension.jsm" to "commit-message".

Another one: https://reviewboard.mozilla.org/r/118390/diff/2-3/ :gerald "Guessing the UI doesn't handle cases where the commit message doesn't actually change between pushes"

Comment on attachment 8845371 [details] mozreview: check if commit message FileDiff is present by parsing page.model (bug 1345740) https://reviewboard.mozilla.org/r/118574/#review120530 ::: commit-message-06154:7 (Diff revision 1) > + > +Commit message FileDiff is not displayed if there was no change between revisions. Currently we don't detect it and page is styled as commit message FileDiff would be displayed. > + > +All FileDiffs which are present on the page are stored in RB.DiffViewerPageModel files attribute. Proposed detection method is checking if any FileDiff id stored there is one of the commitMsgIds properties. > + > +MozReview-Commit-ID: 9KTChjuMWwK To test Create an a review request with at least 3 revisions. There should be no change in commit message from second to third. Then no commit message FileDiff is displayed for diff/2-3/. No commit message styling should be applied.

Comment on attachment 8845371 [details] mozreview: check if commit message FileDiff is present by parsing page.model (bug 1345740) https://reviewboard.mozilla.org/r/118574/#review120586 I can't manually test this at the moment because I'm finalizing the RB upgrade, but will do when I get a workable dev environment. A few nits here though. ::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:142 (Diff revision 1) > $('#review_request').addClass('with-commit-msg-filediff'); > // Set the diff_index's commit message fileName > changeCommitMsgIndexFileName(); > } > > - function detectPageAndChangeCommitMsgFileDiff() { > + function checkFileDiffs() { As tidier practice, maybe we should pass in `page` and `commitMsgIds`? ::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:144 (Diff revision 1) > changeCommitMsgIndexFileName(); > } > > - function detectPageAndChangeCommitMsgFileDiff() { > - var page; > - if ($('#pagination1').children().length == 0) { > + function checkFileDiffs() { > + // Check if any of the files in page's model is a commit msg one. > + var files = page.model.get('files').map(function(k) {return k.id}); semi-colon ;) ::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:146 (Diff revision 1) > > - function detectPageAndChangeCommitMsgFileDiff() { > - var page; > - if ($('#pagination1').children().length == 0) { > - page = 1; > - } else { > + function checkFileDiffs() { > + // Check if any of the files in page's model is a commit msg one. > + var files = page.model.get('files').map(function(k) {return k.id}); > + for (var i = 0; i < files.length; i++) { > + if (commitMsgIdsArr.indexOf(files[i]) !== -1) { I don't think `commitMsgIdsArr` is necessary; can't we just check against `commitMsgIds[files[i]]`?

Comment on attachment 8845371 [details] mozreview: check if commit message FileDiff is present by parsing page.model (bug 1345740) https://reviewboard.mozilla.org/r/118574/#review120608 ::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:143 (Diff revision 1) > // Set the diff_index's commit message fileName > changeCommitMsgIndexFileName(); > } > > - function detectPageAndChangeCommitMsgFileDiff() { > - var page; > + function checkFileDiffs() { > + // Check if any of the files in page's model is a commit msg one. ``` // Check if any of the files in page's model is a commit Message ```

Comment on attachment 8845371 [details] mozreview: check if commit message FileDiff is present by parsing page.model (bug 1345740) https://reviewboard.mozilla.org/r/118574/#review120586 > I don't think `commitMsgIdsArr` is necessary; can't we just check against `commitMsgIds[files[i]]`? I'm checking if a certain `FileDiff` id is a value of any property of `commitMsgIds`. As it might happen few times when user is switching revisions, I've stored these values in an array to simply use `indexOf`. `commitMsgIdsArr` is an array of `commitMsgIds` properties values.

Comment on attachment 8845371 [details] mozreview: check if commit message FileDiff is present by parsing page.model (bug 1345740) https://reviewboard.mozilla.org/r/118574/#review120586 > As tidier practice, maybe we should pass in `page` and `commitMsgIds`? `checkFileDiffs` is a listener of `mr:diff-context-loaded`. Passing anything else than Event would require fork modification. Also, I don't think this method would be used in any other context.

Comment on attachment 8845371 [details] mozreview: check if commit message FileDiff is present by parsing page.model (bug 1345740) https://reviewboard.mozilla.org/r/118574/#review120586 Just to speed up the testing here is how to create a revision without a change in local test-repo (just after ./start-local-mozreview) echo root > foo hg commit -A -m 'root commit - foo added' hg phase --public -r . echo foo > foo hg commit -m 'Bug 1 - Foo modified 1' hg push -y echo foo2 > foo hg commit --amend (do not change anything) hg push -y

Comment on attachment 8845371 [details] mozreview: check if commit message FileDiff is present by parsing page.model (bug 1345740) https://reviewboard.mozilla.org/r/118574/#review121536 ::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:144 (Diff revision 2) > changeCommitMsgIndexFileName(); > } > > - function detectPageAndChangeCommitMsgFileDiff() { > - var page; > - if ($('#pagination1').children().length == 0) { > + function checkFileDiffs() { > + // Check if any of the files in page's model is a commit message. > + var files = page.model.get('files').map(function(key) { return key.id; }); Since you're already looping over all of these items, can you simply do: ```js var files = page.model.get('files').filter(function(file) { return commitMsgIdsArr.indexOf(key.id); }); if(files.length) { showCommitMsgStyling(); } ``` That would cut out the second loop.

Comment on attachment 8845371 [details] mozreview: check if commit message FileDiff is present by parsing page.model (bug 1345740) https://reviewboard.mozilla.org/r/118574/#review121588 Did a quick check and it works great! Just the small suggestion and then good to go!

Comment on attachment 8845371 [details] mozreview: check if commit message FileDiff is present by parsing page.model (bug 1345740) https://reviewboard.mozilla.org/r/118574/#review121606 Awesome! Great work Piotr!

Comment on attachment 8845371 [details] mozreview: check if commit message FileDiff is present by parsing page.model (bug 1345740) https://reviewboard.mozilla.org/r/118574/#review122128 rs

Pushed by mcote@mozilla.com: https://hg.mozilla.org/hgcustom/version-control-tools/rev/5edc2b524db5 mozreview: check if commit message FileDiff is present by parsing page.model r=davidwalsh,mcote